-
Notifications
You must be signed in to change notification settings - Fork 184
chore(ci): automated chore_release PR cascade #18572
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
base: edge
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18572 +/- ##
==========================================
- Coverage 24.98% 23.84% -1.15%
==========================================
Files 3276 3275 -1
Lines 282012 280883 -1129
Branches 33705 33743 +38
==========================================
- Hits 70471 66974 -3497
- Misses 211515 213884 +2369
+ Partials 26 25 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
scripts/chore_release_pr_gen.py
Outdated
# Remove default_branch if present downstream | ||
# This is a clean way to: | ||
# - set the default branch at the end | ||
# - handle someone including the default branch in the branch_list | ||
# - set downstream to [] if target_branch is the default, as it is in most cases | ||
downstream = [b for b in downstream if b != default_branch] | ||
# Append default_branch if target_branch is not already the default | ||
if target_branch != default_branch: | ||
downstream.append(default_branch) | ||
return downstream |
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.
I think I might prefer having the default in the repository variable and not doing any manipulation in the script.
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.
the default branch is actually the github repo option default branch, which you can retrieve from repo properties - then we don't even have to put it in the repo args
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.
I think I prefer it in the repo variable as the source of truth of the branch cascade vs understanding rare knowledge about the default branch of the repository as always the last element.
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.
couple little things but i think the important one is getting the full pr body in the created branches
scripts/chore_release_pr_gen.py
Outdated
"""Splits a comma-separated branch list into a clean Python list.""" | ||
return [b.strip() for b in branch_list_str.split(",") if b.strip()] | ||
|
||
def get_downstream_branches(branch_list: List[str], target_branch: str, default_branch: str = DEFAULT_BRANCH) -> List[str]: |
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.
not really a thing for a one-off script like this but you usually want default values in python to be None
- since python name binds by reference, if you were to do default_branch.strip()
and default_branch
was bound to DEFAULT_BRANCH
, the string in DEFAULT_BRANCH
would be persistently modified.
scripts/chore_release_pr_gen.py
Outdated
# Remove default_branch if present downstream | ||
# This is a clean way to: | ||
# - set the default branch at the end | ||
# - handle someone including the default branch in the branch_list | ||
# - set downstream to [] if target_branch is the default, as it is in most cases | ||
downstream = [b for b in downstream if b != default_branch] | ||
# Append default_branch if target_branch is not already the default | ||
if target_branch != default_branch: | ||
downstream.append(default_branch) | ||
return downstream |
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.
the default branch is actually the github repo option default branch, which you can retrieve from repo properties - then we don't even have to put it in the repo args
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.
Nice! Looks good to me!
import sys | ||
|
||
if len(sys.argv) > 1 and sys.argv[1] == "test": | ||
unittest.main(argv=[sys.argv[0]]) |
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.
oh this is cool, i like it
--base "$branch" \ | ||
--head "${{ github.event.pull_request.head.ref }}" \ | ||
--title "${{ github.event.pull_request.title }} into $branch" \ | ||
--body "${{ github.event.pull_request.body }} \ |
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.
Is it worth mentioning the original PR number somewhere in the new PRs, so that future readers know how they're related?
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.
Yes I think I will add it at the top of the body.
for branch in $downstream_branches; do | ||
pr_url=$(gh pr create \ | ||
--base "$branch" \ | ||
--head "${{ github.event.pull_request.head.ref }}" \ |
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.
Hm, I'm not so familiar with Github's model, but does this mean that as far as Github knows, the same author's branch is going to be used for all the PRs?
What happens if the code needs to be tweaked differently for the different downstream_branches?
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.
Yes, the same branch is used and the author will be expected to create a commit or commits to resolve conflicts and tweak as needed. The PR(s) are the TODO to make sure that gets done. As discussed if we move to normal merges, folks will need to be careful to preserve all the commits as they proceed through the PRs.
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 I have now remarked in the epic slack thread, if we are normal merging, I do not see this automation as helpful in its current form.
Overview
edge
as the default branch and terminating downstream branch.