KAFKA-15201: Allow git push to fail gracefully #14645
Conversation
release.py
Outdated
| @@ -730,7 +730,7 @@ def select_gpg_key(): | |||
| fail("Ok, giving up") | |||
| if not user_ok("Ok to push RC tag %s (y/n)?: " % rc_tag): | |||
| fail("Ok, giving up") | |||
| cmd("Pushing RC tag", "git push %s %s" % (PUSH_REMOTE_NAME, rc_tag)) | |||
| cmd("Pushing RC tag", "git push %s %s" % (PUSH_REMOTE_NAME, rc_tag), num_retries=0) | |||
There was a problem hiding this comment.
Do we want to have allow_failure=True here too?
There was a problem hiding this comment.
if we put allow_failure=True, when git push failed, the program will continue to run the remaining lines after git push instead of failing gracefully. So I think we should just keep num_retries=0 so that it triggered the fail function and proceed to do the cleanup
There was a problem hiding this comment.
If we don't pass this argument, the default for num_retries is already 0.
Line 103 in fa36a7f
So, I assume that this line change does not really change anything?
There was a problem hiding this comment.
Can I ask like when git push failed, do we expect the program to perform clean up like the logic defined in the def fail ? Or the program can directly exit with error ?
The current infinite loop lies in the fact that the API def cmd calls def fail , which in turn calls back def cmd, and hence the program never exits. If we can exit directly with error upon failed git push, the implementation can be a lot simplified.
There was a problem hiding this comment.
Unfortunately, I don't have the logs now and it will take me time to reproduce.
I agree with your assessment on the cmd/fail loop. Perhaps, we can detect that the failure is due to git and in such cases, mark delete_gitrefs=false. That will ensure that fail() does not call git commands again.
There was a problem hiding this comment.
I think the original implementation is that whenever cmd fails, the clean-up logic will always be fired (i.e. switch to the original working branch, delete the release branch & the tag). If we add the git-failure check, we may lose such feature.
I've modified the git push part to avoid using the cmd function (thereby avoiding the cmd/fail loop). Could you take a look and leave me some feedback ?
The proposed logic is that when git push fails, the program will exit gracefully without cleaning the release branch & tag (so the user will have to manually clear them in order to restart)
|
@divijvaidya Can I get your feedback for this PR ? Many Thanks |
release.py
Outdated
| @@ -730,7 +730,7 @@ def select_gpg_key(): | |||
| fail("Ok, giving up") | |||
| if not user_ok("Ok to push RC tag %s (y/n)?: " % rc_tag): | |||
| fail("Ok, giving up") | |||
| cmd("Pushing RC tag", "git push %s %s" % (PUSH_REMOTE_NAME, rc_tag)) | |||
| cmd("Pushing RC tag", "git push %s %s" % (PUSH_REMOTE_NAME, rc_tag), num_retries=0) | |||
There was a problem hiding this comment.
If we don't pass this argument, the default for num_retries is already 0.
Line 103 in fa36a7f
So, I assume that this line change does not really change anything?
release.py
Outdated
| cmd("Resetting repository working state to branch %s" % starting_branch, "git reset --hard HEAD && git checkout %s" % starting_branch, shell=True, allow_failure=True) | ||
| cmd("Deleting git branches %s" % release_version, "git branch -D %s" % release_version, shell=True, allow_failure=True) | ||
| cmd("Deleting git tag %s" %rc_tag , "git tag -d %s" % rc_tag, shell=True, allow_failure=True) |
There was a problem hiding this comment.
If the first line fails which is correctly switching the branch, we will proceed with next line and end up deleting an incorrect branch. We don't want that. Am I missing something?
|
@divijvaidya shall we merge this PR ? Thanks |
|
@divijvaidya Should we merge this PR? |
Reviewers: Divij Vaidya <[email protected]>, Qichao Chu <[email protected]>
Reviewers: Divij Vaidya <[email protected]>, Qichao Chu <[email protected]>
Reviewers: Divij Vaidya <[email protected]>, Qichao Chu <[email protected]>
As described in the ticket KAFKA-15201, this PR allows the
git pushaction to fail gracefully in case of error.Committer Checklist (excluded from commit message)