Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete a remotely created project if runtime setup fails. #2725

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Aug 22, 2023

BugDX-2093 init project with `--language [email protected]` cause actual creation of the project in the platform

@mitchell-as mitchell-as requested a review from daved August 22, 2023 20:38
@mitchell-as mitchell-as marked this pull request as ready for review August 22, 2023 20:38
@mitchell-as
Copy link
Contributor Author

@daved ping

@daved
Copy link
Contributor

daved commented Aug 24, 2023

@Naatan Should the deletion occur when there is any error after platform project creation?

logging.Debug("Deleting remotely created project due to runtime setup error")
err2 := model.DeleteProject(namespace.Owner, namespace.Project, r.auth)
if err2 != nil {
multilog.Error("Error deleting remotely created project after runtime setup error: %v", errs.JoinMessage(err2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be returned to the user. It means they're in a broken state and they should be made aware of it.

@Naatan
Copy link
Member

Naatan commented Aug 24, 2023

@Naatan Should the deletion occur when there is any error after platform project creation?

The AC says "when the runtime fails to source"; so no not any error, just runtime errors. Seems the code as is addressed the AC, although it should still communicate failure of deletion to the user.

@daved
Copy link
Contributor

daved commented Aug 24, 2023

@Naatan The reason I asked is because I thought the underlying purpose was to ensure that the remote and local states were effectively equal. Deleting only during runtime handling seems to leave a window open for the disparity to occur.

@Naatan
Copy link
Member

Naatan commented Aug 24, 2023

@daved What other conditions do you think we should consider? The ACs are pretty clear. I'm not against expanding the scope if we find other cases, but this discussion feels very open-ended at the moment. Are you seeing a particular issue, or are you just putting out feelers?

@daved
Copy link
Contributor

daved commented Aug 25, 2023

@Naatan The errors that can happen after a project is first created on the platform are from:

  • branch, err := model.DefaultBranchForProject(platformProject)
  • err = model.UpdateProjectBranchCommitWithModel(platformProject, branch.Label, commitID)
  • err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, proj, commitID, true, target.TriggerInit, r.svcModel)

This PR will remove the created project from the platform for the last behavior only, but there are two steps prior that could leave the user in an undesirable workflow.

@Naatan
Copy link
Member

Naatan commented Aug 25, 2023

I see.. I don't think it's worth expanding the check to those necessarily. While those are API's that can of course give an error, the error condition would only happen in unexpected scenario's, in which case I'd rather not go deleting projects.

For the runtime scenario, we know what this error scenario means, so acting on it with intend is perfectly reasonable.

@mitchell-as mitchell-as merged commit a42a0d4 into version/0-41-0-RC1 Aug 25, 2023
6 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-2093 branch August 25, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants