-
Notifications
You must be signed in to change notification settings - Fork 502
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
Add changelog remote lookup feature #1003
Add changelog remote lookup feature #1003
Conversation
Had to rebase again :) |
We now correctly assume that a non-patch final version needs to have a remote changelog available and failure if this one does not exist. We also have to provide the target tag to the changelog subcommand to be able to find the right release notes generation approach. The git package has been adapted to contain a more robust way to retrieve tags for a dedicated branch. Signed-off-by: Sascha Grunert <[email protected]>
Ready for review, PTAL @justaugustus @hoegaarden @hasheddan @cpanato |
notesOptions.Branch = branch | ||
notesOptions.StartRev = startRev | ||
notesOptions.EndRev = endRev |
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.
Are these required for notesOptions.ValidateAndFinish()
? Wondering if some of these options being set in this list could either be passed as parameters to NewOptions()
or configured as sane defaults. What do you think?
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.
Yes exactly. Yeah we could apply something like a builder pattern for validation... 🤔
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 is exactly what I was thinking :) Given that this does not change functionality, would you prefer for this to be tackled in a follow-up PR?
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 would like to defer that to a follow-up PR if you don't mind :)
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.
SGTM!
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
/hold for more reviews
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 for this great work @saschagrunert!
/lgtm
Thanks @saschagrunert! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, hasheddan, 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 |
🎉 With this change we're pretty close to be able to replace the relnotes script and parts of anago with the
krel changelog
subcommand. Just oneTODO
left. 😇Description
We now correctly assume that a non-patch final version needs to have a
remote changelog available and failure if this one does not exist.
We also have to provide the target tag to the changelog subcommand to be
able to find the right release notes generation approach.
The git package has been adapted to contain a more robust way to
retrieve tags for a dedicated branch.
Demo
All three runs will be merged together in one single
CHANGELOG-1.16.md
file, which looks like this:https://gist.github.com/saschagrunert/b04e87ebfd7d4e632f91ecd61ed478b7#kubernetes-v1160-release-notes