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

Temporarily restore PR branch, if deleted, in uplift.py #19683

Closed
wknapik opened this issue Nov 23, 2021 · 11 comments · Fixed by brave/brave-core#14175
Closed

Temporarily restore PR branch, if deleted, in uplift.py #19683

wknapik opened this issue Nov 23, 2021 · 11 comments · Fixed by brave/brave-core#14175

Comments

@wknapik
Copy link
Contributor

wknapik commented Nov 23, 2021

When a PR is merged, the branch is automatically deleted. If an uplift is attempted afterwards, it will fail, because the PR branch is gone. I added a note on the wiki about restoring it manually, but ideally there would be no manual steps needed, except calling the script.

This feature could be behind a command-line option. We'd certainly use that option in the brave-core-create-uplift-prs pipeline.

@wknapik
Copy link
Contributor Author

wknapik commented Nov 23, 2021

cc @bsclifton as top contributor to the script

@bsclifton
Copy link
Member

bsclifton commented Nov 23, 2021

@wknapik I didn't think the branch name was ever used? The script will look up the original pull request by the pull request number and then (using GitHub API) will get the SHA that was merged into master basically. The merge commit itself is what gets uplifted

There is a problem if you need to uplift in other scenarios though; like once it's already in master and beta, trying to use the script to get into release will fail. I think I've captured a general "harden uplift script issue"

edit:
whoops; maybe not? I just found #14656

@wknapik
Copy link
Contributor Author

wknapik commented Nov 24, 2021

@bsclifton this is easy to reproduce. Every time I've tried using the uplift pipeline, which just calls the script, it failed when the PR branch was deleted and succeeded when it was restored. I might be doing something wrong, but there isn't much to mess up here...

@bsclifton
Copy link
Member

@wknapik do you have a link to a failed job? I uplift PRs all the time which are already merged and branch is deleted. The PR number is used w/ GitHub's API to retrieve meta data; the branch itself is never referenced AFAIK

@wknapik
Copy link
Contributor Author

wknapik commented Nov 29, 2021

@bsclifton I went through the build history for this pipeline and it seems my builds are out of our retention window. Next time I run this, I'll try to remember to add a comment here.

@mihaiplesa I think you've also experienced breakage when the source branch is deleted, right?

@bsclifton
Copy link
Member

I guess I'd just like to see a case where it does break? Basically, brave-core is setup to auto-delete the branch after merge. I've never had a branch existing when running the tool

I have had problems where the tool will fail if the branch is NOT merged and NOT deleted. Maybe that's what we were thinking of?

@bsclifton
Copy link
Member

Awesome, thanks for sharing a case! 😄 Wonder why this doesn't happen all the time? Maybe something changed on GitHub side about how they prune the remote. One solution would be to grab the merge commit instead (and uplift that)

@wknapik
Copy link
Contributor Author

wknapik commented Dec 7, 2021

In my experience this happens a 100% of the time, I think. I don't do uplifts that often.

I was thinking maybe it's about something I'm doing differently, so I waited for someone else to run into this.

As for looking at the merge commit - sounds good.

@mihaiplesa
Copy link
Contributor

@mihaiplesa mihaiplesa added this to To do in CI concerns Dec 28, 2021
@wknapik
Copy link
Contributor Author

wknapik commented Feb 1, 2022

@mihaiplesa we could add triggering the uplift pipeline (and deleting branches at the right time) based on a label on PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
CI concerns
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants