-
Notifications
You must be signed in to change notification settings - Fork 83
ci autobumper: Fix change log in the PR body in case of a major version jump #839
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
Conversation
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.
Your PR contains a merge commit and duplicate commits, please clean this up so that the PR has a single, up-to-date commit.
Regarding the change itself, just truncating the output is not correct since the output may contain Markdown / HTML which is then no longer valid. Maybe we can either compact / de-duplicate the release notes (many versions will contain the same, cherry-picked fixes) a little bit or truncate the actual content to not risk broken formatting?
Edit: looking around the code a bit I think this is a bug, this is the core loop:
haproxy-boshrelease/ci/scripts/autobump-dependencies.py
Lines 390 to 397 in 500c371
| for line in file: | |
| if (line.endswith(str(latest_version)+"\n")): # Start copying from latest version head | |
| startCopy = True | |
| if (line.endswith(str(current_version)+"\n")): # Stop when reaching current version | |
| break | |
| if not startCopy: | |
| continue | |
| releaseNote += line |
It searches for the target version line that looks like this 2025/10/23 : 3.2.7 and then the start version (2.8.16) in the same format. But since we are now reading the changelog for 3.2 you will find that it only contains version 2.8.0 and then immediately moves on to 2.9-dev0 due to the way HAProxy maintains versions in dedicated repositories. In that case the auto-bumper will just dump the entire version history into the PR which obviously is way too much.
We need to fix that so that when a major or minor jump is detected it starts from the start versions .0 (2.8.0 in this case).
|
about truncation: I think it is fine to truncate the text extracted from the release note itself to some reasonable length, e.g. 50k. |
Thanks, Max @maxmoehl. Please review a proposal. The squash will be performed as usual at the end after the review success. |
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 comparison of changelog lines against normalized (i.e. stripping -dev) versions will not work. Please check.
Some other improvements for easier readability suggested.
ci/scripts/autobump-dependencies.py
Outdated
|
|
||
| # Returns normalized versions for changelog comparison | ||
| def _get_comparison_versions(self, current_version, latest_version): | ||
| comparison_current = str(current_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.
is current_version not already a string? why the conversion?
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.
No, it is Version.version, see class definition:
class Release:
"""
A specific release (i.e. version) of a dependency.
Currently, only the latest release of each dependency is fetched.
"""
name: str
version: version.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.
But ... why not use it as a version then? Now you are taking an already parsed version, formatting it as a string and then splitting it again.
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.
You need to convert version to string to split 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.
Why are you not writing the function like this?
def _get_comparison_versions(self, current_version, latest_version):
"""
HAProxy tracks each minor version in a dedicated repository and the changelog in each
repository only ever observes the .0 of previous minor versions as they are split off into
a dedicated repository at that point. To make a comparison across minor versions we need to
compare against the .0 of the previous minor version, this helper converts the comparison
base accordingly.
"""
if latest_version.major > current_version.major or latest_version.minor > current_version.minor:
current_version = version.Version(f"{current_version.major}.{current_version.minor}.0")
return str(current_version), str(latest_version)No need to stringify and split the version. I also noticed that this is currently does not need to be a method, so I'd either make it _get_comparison_versions(self) and read the versions from self or turn it into a function that is not attached to the class.
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.
Thanks @maxmoehl , moreover, the Version class is comparison aware, so that I could also remove the unnecessary conversions to int. Took over your proposal.
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.
LGTM. Let's see it in action.
|
@Dariquest, the request for a squashed set of commits (one or more, as much as makes sense) still remains. Especially the "merge master" commit is something we don't want for a linear history. |
Sure, thanks again, it does not make sense to squash before other changes are all in. |
b315eb8 to
96d154a
Compare
ci/scripts/autobump-dependencies.py
Outdated
|
|
||
| # Returns normalized versions for changelog comparison | ||
| def _get_comparison_versions(self, current_version, latest_version): | ||
| comparison_current = str(current_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.
But ... why not use it as a version then? Now you are taking an already parsed version, formatting it as a string and then splitting it again.
f39ed6d to
8c3274d
Compare
8b20b89 to
5c42778
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.
approving for CI
no, the changes were not pushed yet.
they were, but I'd need some time to actually look into this. More changes than expected.
changes I thought were there for CI were not pushed yet.
Autobump-dependencies concourse pipeline is failing with the error "body is too long (maximum is 65536 characters)" in the create_pr method:
pr = self.remote_repo.create_pull(...GitHub Issue