-
Notifications
You must be signed in to change notification settings - Fork 496
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
Use krel changelog instead of relnotes bash script #1044
Conversation
@saschagrunert: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Soooo good! 🎉
There are just a couple of details I am not sure about, but those should be hashed out easily.
anago
Outdated
"Add/Update $CHANGELOG_FILE for $RELEASE_VERSION_PRIME." \ | ||
|| return 1 | ||
fi | ||
logrun go run ./cmd/release-notes/main.go \ |
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 am not sure if we are in the right path here? I think we are in the repo where k/k
is checked out and not where k/release
is checked out. I might be wrong though.
One way to help with that might be doing a similar thing as with blocking-testgrid-tests
, which is:
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.
Yeah I think this wrong (it's also the wrong tool, whoops 🙈). I think we should have krel in path when running anago. Do we really want to add it to the staging step?
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 really want to add it to the staging step?
Well, I am not sure when the changelog generation really happens, I guess on release
?
Anyhow, when we need a thing for staging (e.g. blocking-testgrid-tests
) we should compile it first thing when we run staging
, when we need a thing for the releasing (e.g. krel
?) we should compile it first thing for release
.
I think this is perfectly fine (for now). What are your concerns with that?
On the long run / ideally we shouldn't compile any of our tools on any of those GCB steps. When we have a new commit on k/release
and all the test go green, we should automatically compile the things and bake a new container image & have that flow through the promotion pipes. The stage
& release
(and other) steps would just use :latest
(or :latest-1.13
if we need that, or what have you ...) of said container image and have all the tools there. But that probably needs a lot of work still, esp. regarding testing, breaking apart anago
, getting rid of all/most shelling-out, ...
At least that is my train of thought ...
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.
Alright, this makes all sense to me. I think I will propose a different PR for building krel in GCB.
This replaces the critical part of anago with the `krel changelog` subcommand. Signed-off-by: Sascha Grunert <sgrunert@suse.com>
This is ready for review now, I rebased on top of the latest master branch and tested it locally. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Running a mock stage of this on |
It finally happens: This PR replaces the relnotes part of anago with the
krel changelog
subcommand./hold
Needs #1043, #1042, #1046