-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix: checking out a non-existing branch fails the project sync #1122
base: main
Are you sure you want to change the base?
Conversation
Need to distinguish a git checkout failure is due to a non-existing branch/tag/commit, or due to an empty repository. For later case we treat the project sync completed but with an error message. AAP-29144: Project sync should report status as "failed" when using non-existent branch
@bzwei You will add pytest for the change, right? |
yes, new tests will be added to eda_qa |
else models.Project.ImportState.FAILED | ||
) | ||
project.import_state = models.Project.ImportState.COMPLETED | ||
project.import_error = str(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense to set an "import error" if the state is not error. Here I see a potential inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the SU. This field should be renamed in another issue when it is time to change UI together.
if "Project folder is empty" in e.args[0] | ||
else models.Project.ImportState.FAILED | ||
) | ||
project.import_state = models.Project.ImportState.COMPLETED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be this failed? in the description you say:
This fix identifies the first case and set the state to be failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScmEmptyError is for the second case where the repo is truly empty. We want it to be completed.
Quality Gate passedIssues Measures |
Need to distinguish a git checkout failure is due to a non-existing branch/tag/commit, or due to an empty repository. The raw error messages are very similar but we have set the project sync state to completed for the later case in an earlier fix. This fix identifies the first case and set the state to be failed.
AAP-29144: Project sync should report status as "failed" when using non-existent branch
Also refactor the existing code to use a proper error class instead of. an error message to identify the case for an empty repo.
Will add e2e tests for the two cases.