-
Notifications
You must be signed in to change notification settings - Fork 11
CLOUDP-295785 - Calculate next version and release notes script #193
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: master
Are you sure you want to change the base?
Conversation
f705997
to
23e1224
Compare
1052aae
to
846787e
Compare
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 haven't reviewed everything, but I've tried the workflow with creating release notes and rendering it. Works really well. The only thing I noticed is that "Other Changes" section doesn't render (see a comment below with the reason).
64f2aa8
to
d55f322
Compare
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.
Awesome changes, and tests 👏
d55f322
to
0d02f6f
Compare
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 got a few questions, nits and suggestions. But overall I think it looks great.
# Traverse added Diff objects only (change type 'A' for added files) | ||
for diff_item in diff_index.iter_change_type("A"): |
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.
Could you please add a more detailed comment here or a docstring to the function explaining how this works? E.g. why we only getting added files? How do we account for modified changelog entries (e.g. added in one commit, modified in another)? Deleted?
This will be extremity helpful for the future us who will be maintaining the tool/making changes here.
- '{", ".join(BREAKING_CHANGE_ENTRIES)}' for breaking change entries | ||
- '{", ".join(FEATURE_ENTRIES)}' for feature entries | ||
- '{", ".join(BUGFIX_ENTRIES)}' for bugfix entries | ||
- everything else will be treated as other entries""", |
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'm looking at the tests: e.g. 20250520_docs_update_readme
and 20250610_refactor_codebase
.
And I think we should make other
to be explicit and reject anything else.
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 deliberately wanted to have some freedom in naming other
kind of changes. The name other
does not tell much, so if somebody wants to use docs
or config
they can and it will just land under Other changes
.
Do you feel there is something wrong with this idea?
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.
With having random strings treated as other
it is hard to tell if it is a real kind or not. Unless you remember implementation, it is hard to say in which section it will be rendered. E.g. will 20250520_docs_update_readme
be rendered in features or in other? Or perhaps it is a prelude?
I think kind in file names and kind in front matter must always be consistent and explicit. This means that:
- Let's not allow random strings in file names and in
kind
in front matter. - If one kind has multiple aliases (e.g.
major
andbreaking
) - let's choose one thing (e.g.major
) as a canonical name and use it as a kind in both filenames and front matter.
So consistency in the data layer (files in our case), but flexibility on the client (CLI tool).
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 agree that the kind in filename and in the file contents itself has to be identical. We are right now only checking if they map to the same ChangeKind value. I can easily change this and compare actual string representations.
Unless you remember implementation, it is hard to say in which section it will be rendered. E.g. will 20250520_docs_update_readme be rendered in features or in other? Or perhaps it is a prelude?
I don't find this true. The information about the mapping is presented in the CLIhelp
of the method that creates a changelog file. I agree if we have less values to choose from it will be easier to memorize them and also see where the change will land in Release Notes just by scanning the filenames.
So you're proposing naming the same as the sections in the Release Notes template:
- breaking
- feature
- fix
- other
?
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 don't find this true. The information about the mapping is presented in the CLI
help
of the method that creates a changelog file.
Ok, I can say "unless you remember the implementation or look it up in the --help
command". And still you need to read a multiline help string and catch "everything else will be treated as other entries".
So you're proposing naming the same as the sections in the Release Notes template:
I don't think it necessary has to be exact match with section names, but I would really want to have 1:1 mapping between sections and kind
s in files. And I want the number of kind
options to be deliberately very limited. Like prelude
, major
, feature
, fix
, other
and that's it.
If we can make it dead simple we won't even need CLI to create change notes or read about how it works – just copy-paste existing one and modify it. Only read help if it fails to render (e.g. in CI). With limited number of options we can make it like so.
Co-authored-by: Mikalai Radchuk <[email protected]>
Co-authored-by: Mikalai Radchuk <[email protected]>
Co-authored-by: Mikalai Radchuk <[email protected]>
repository_path: Path to the Git repository. Default is the current directory '.'. | ||
changelog_sub_path: Path to the changelog directory relative to the repository root. Default is '{DEFAULT_CHANGELOG_PATH}. | ||
initial_commit_sha: SHA of the initial commit to start from if no previous version tag is found. | ||
initial_version: Version to use if no previous version tag is found. Default is '{DEFAULT_INITIAL_GIT_TAG_VERSION}'. |
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 see you updated args in the function and removed defaults, but defaults are still mentioned here.
Summary
Initial work for calculating next operator version and release notes. This is based on https://docs.google.com/document/d/1eJ8iKsI0libbpcJakGjxcPfbrTn8lmcZDbQH1UqMR_g/edit?tab=t.aov8hxoyxtos#heading=h.6q8mwv95pdu6.
This is not yet integrated with CI nor forces engineers to create change log files. There will be next smaller PRs that will address missing integration. Functionalities added by this PR:
scripts.release.calculate_next_version.py
-> this will calculate next version based on the changelog files provided.scripts.release.create_changelog.py
-> this is a tool for creating changelog files. It makes life easier for engineers and reduces a chance for errors in filename and contents syntaxscripts.release.release_notes.py
-> create release notes based on the changes since last release tagProof of Work
Passing unit tests for now is enough.
Checklist
Reminder (Please remove this when merging)