-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
Update changelog/contributors/version bump in one script #19379
Conversation
title=title, | ||
body="\n\n".join([BASE_PR_BODY, formatted.internal]), | ||
assignee=release_manager, | ||
labels=["automation:release-prep"], |
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.
Note branch name and this label name, per the discussion of conventions in the PR description.
# infer the changes since the most recent previous release on this branch | ||
prior_tag = git("describe", "--tags", "--abbrev=0", release_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.
This changes this script to not need the --prior
argument, and instead just look for the most recent tag before the release ref.
"--log-level", | ||
default="WARNING", |
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.
For interactive use WARNING
seems fine, but for running on CI, I was planning to set --log-level=DEBUG
or something, to make it easier to understand what it's doing.
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.
Some drive by comments before I review thoroughly
|
||
logger = logging.getLogger(__name__) | ||
|
||
BASE_PR_BODY = softwrap( |
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.
Can this instead be a comment on PR? Making it the PR body feels weird
Another drive by observation without having looked at the code. Given the number of steps done by this script, I think I would appreciate it if I were able to specify which ones to run. Say it gets halfway through and then aborts due to some error/issue or what not. If I'm able to address that and re-run I'd like it to be idempotent (either by detection, or perhaps more likely by me asking it to proceed at a certain step or something along those lines). As long as the actions are local only, there's always the option to revert any changes to start over, but in case the issue is to manually tweak and then continue in order to get over a hump that could prove difficult. This is of course only a nice to have, but if it's easy enough to add.. perhaps worth having in the back of your mind at least :) Sorry for the long rambling.. |
Piggybacking off @kaos maybe we can expose one script per step, that can each be run independently (with assumptions, of course). |
Sounds good. I personally prefer calling Python functions in Python rather than arranging python subprocesses in bash, so I'll go with @kaos's original idea of being able to specify which action as args to the script. |
Generally, I see exposing each script's main as I can speak personally that my ability to reason about code goes down as line count goes up (I get easily distracted) so fewer, smaller, files makes me 😍 |
I think having it be idempotent would be best, and preferable to a bunch of one-function scripts. But note that this is really |
I think if the cherry pick script detects a CP with the proper label, it can do the version bump for the milestone branch. |
I'll switch to idempotent. 👍
I think we'll want dedicated automation for this, since it both does than the current auto cherry picker script:
Of course, this non-cherrypicker automation can and should still do the version bump, if it needs to 👍 |
I think we'd always want a human to create the tag? Esp. since we sign the release tags. And we need some human-triggered action to kick off a release. But scripts can then act off the tag to automate everything else. |
@huonw what's the state of this PR? 😄 |
Updates:
Example: huonw#6 (particularly huonw@894071b) |
Can you comment a smidge on switching from From the CI perspective, both are sufficient so that's unlikely the razor. From the local perspective, I imagine it's much more likely a user has |
# Only used for release management | ||
PyGithub==2.0.0rc1 |
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.
We can enforce this, by adding ("[PyGithub]", "src/python/pants_release/**", "!*"),
to
Lines 36 to 49 in 00786d3
__dependents_rules__( | |
( # Only the explorer server may depend on these libraries | |
( | |
"[fastapi]", | |
"[starlette]", | |
"[strawberry-graphql]", | |
"[uvicorn]", | |
), | |
"src/python/pants/explorer/server/**", | |
"!*", | |
), | |
# Free for all on the rest | |
("*", "*"), | |
) |
Something like:
__dependents_rules__(
( # Only the explorer server may depend on these libraries
(
"[fastapi]",
"[starlette]",
"[strawberry-graphql]",
"[uvicorn]",
),
"src/python/pants/explorer/server/**",
"!*",
),
("[PyGithub]", "src/python/pants_release/**", "!*"),
# Free for all on the rest
("*", "*"),
)
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 a slight aside, should we swap so the core pants package is includelisted, and everything else is free game?
Because it shouldn't matter if a release needs fastapi
(I mean why, but you get the point) or some non-included plugin uses PyGitHub
(what if its a GitHub plugin?)
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've filed #19458
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 a slight aside, should we swap so the core pants package is includelisted, and everything else is free game? Because it shouldn't matter if a release needs
fastapi
(I mean why, but you get the point) or some non-included plugin usesPyGitHub
(what if its a GitHub plugin?)
That works.
It was motivated by two things:
Good catch! Using |
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.
Overall LGTM. I love me some more automation.
cur_version_sha, prev_version_sha = git( | ||
"log", "-2", "--pretty=format:%h", "src/python/pants/VERSION" | ||
"log", "-2", "--pretty=format:%h", str(VERSION_PATH) | ||
).splitlines(keepends=False) |
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.
do we want to extract this into a method in git.py to keep all the git in one place?
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.
For one-off/specialised calls like this, I don't have a particular point of view. For me, I'd lean away from wrapping this sort of thing up into a "find_two_most_recent_commits_changing_version
" function unless there were more locations that needed it! 🤷
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.
That's fair. There's always a tension between keeping everything at the same abstraction level and having a pile of hyper-specific functions. I find that naming the function by why I would use it helps me determine if it's worth wrapping. (Put another way: the view of this interface by the consumer rather than the provider.) something like get_commit_filter_for_this_release
would fit more naturally in a list of steps that this function performs, like:
- get git commit filter for this release
- get and sort all contributors from that range
- identify new contributors
- ...
You're right that this would make our git.py contain a lot of one-off functions.
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 this has been open long enough and I trust your judgement on fixing the comments. So I'll go ahead and approve, because it's good stuff.
8af8b04
to
f0e56b1
Compare
I've created the |
This evolves the
changelog.py
script intostart_release.py
, which does the first 4-ish steps of the release process:CONTRIBUTORS.md
VERSION
, onmain
This is currently just meant for running locally, but a possible next step would be packaging it up to run in CI: a manually triggered workflow that takes the new version number as an input and then runs
.../start_release.py --new $version_input --release-manager $sender_login --publish
, to open the PR as @WorkerPants.This is a step towards #19279, and in particular, does most of "1", other than having it run in CI.
This proposes a few conventions:
automation/
, e.g. these branches areautomation/release/2.34.56
automation:
, e.g. these release prep PRs would be created with labelautomation:release-prep
Thoughts? We could retrofit these conventions onto the cherry-picker, if they seem nice.
Next steps after this might be: