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
Make sure release has a disttag #2106
Make sure release has a disttag #2106
Conversation
Build failed. ✔️ pre-commit SUCCESS in 1m 33s |
Build failed. ✔️ pre-commit SUCCESS in 2m 00s |
Build failed. ✔️ pre-commit SUCCESS in 2m 02s |
One thing to note, if the users sets a manual dist tag like |
Build failed. ✔️ pre-commit SUCCESS in 2m 01s |
Build succeeded. ✔️ pre-commit SUCCESS in 1m 36s |
Build succeeded. ✔️ pre-commit SUCCESS in 1m 37s |
Thanks it is #1978. It was closed that's why I didn't find it |
# Make sure the new release has a dist tag if it is not already included explicitly | ||
# (in release) or implicitly (in up.specfile.raw_release) | ||
if ( | ||
release | ||
and not release.endswith("%{?dist}") | ||
and not self.up.specfile.raw_release.endswith("%{?dist}") | ||
): | ||
release = f"{release}%{{?dist}}" |
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.
looks ok to me, but I am not that familiar with this code, maybe @majamassarini @nforro could you take a look please?
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 code looks good to me but if I get it correctly we are changing the package name format and I think some user may depend on the old format (somehow in their script) and we should be prepared to deal with some issue.
I hope I am wrong.
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.
Indeed that's the case. It will append %{?dist}
if it sees that the final form of the release does not include it, including if they use specific templates and suffixes, as long as update_release: true
. My assumption here is that if they had a non-standard format, they would be using update_release: false
and hardcode the release tag rather than have a templatized form.
But I am not sure how common release_suffix
is used, so I will deffer the judgement if it should be gated through an option or add it with appropriate change_notes and deal with any issues that pop up.
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
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
78ead16
to
c276327
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 1m 46s |
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 51s |
fd13a16
into
packit:main
This one depends on a few upstream PRs: - packit/tmt-plans#8 - packit/packit#2106 --------- Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Not sure if this is the most optimal place to put it
TODO:
Fixes #1978
RELEASE NOTES BEGIN
Enforce dist tag
%{?dist}
when updating releaseRELEASE NOTES END