-
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 support for using arbitrary Go template when generating markdown. #1008
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @j0n3lson! |
Hi @j0n3lson. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @saschagrunert |
/check-cla |
@j0n3lson: GitHub didn't allow me to request PR reviews from the following users: lynncyrin. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
In case it's helpful for +1 person reinforcing this point: me and @saschagrunert are looking to use the k8s release notes generator for |
/check-cla |
Awesome, thanks. 🙏 This will help us to spread the tool around the world. @j0n3lson can you please sign the CNCF CLA here. /ok-to-test |
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.
Generally LGTM, I would recommend in going from markdown template to go template file to have it a bit more generic. Later on we could also add support for template strings, but this seems out of scope for now.
/assign @justaugustus @hoegaarden @cpanato @hasheddan asking kindly for review |
I was reminded of this while writing up my most recent release 😄 urfave/cli#1077 |
I'll try to get back to this FR this week. Thanks for being patient with me
:)
|
Thanks @j0n3lson! Keep us posted. :) |
Quick update:
Working on:
Thinking about:
Hoping to finish up with this soon. Sorry for the long wait :) |
Could you have a look. I updated the reviewer notes with some specific questions I had. My test are failing because |
We could either reduce the cyclomatic complexity (I can do that as a follow up as well) or you add something like this to linters-settings:
gocyclo:
min-complexity: 35 |
// TODO: Not sure these options are guaranteed to be set but we need | ||
// them in rendering. Perhaps these should be set in CreateDocument()? | ||
doc.PreviousRevision = opts.StartRev | ||
doc.CurrentRevision = opts.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.
I was thinking about something like this too, we couple a var to two different functionalities (rendering and revision retrieval), which seems kinda bad. Let's stick to the TODO for now and follow-up later on this.
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 think I answered my own question about whether they're guaranteed to be set:
The default value for Options.DiscoverMode is RevisionDiscoveryModeNONE which means that either or both Options.StartRev and Options.EndRev can be empty.
With existing implementation will see an error if Options.ReleaseTars is given and the above is true (createDownloadsTable() will raise the error). This revision maintains that behavior though (FetchMetadata() will raise the error instead).
Bumped the min-complexity per your suggestion. |
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
PTAL @justaugustus @hasheddan |
@j0n3lson -- Can you remove that change and instead add: //nolint:gocyclo above the function? Average cyclo is much lower and we should encourage less complex functions by having the linter fail. |
In retrospect, this has been open for a while, so let's merge and iterate. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: j0n3lson, 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 |
👀 🚀 ! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds the ability to render notes in markdown using a user-supplied template. A default template is supplied with this PR and is used by default when neither a template string or template file are specified.
This makes the tool less opinionated about the output format and gives the user a bit more control over the output. Wanting to customize the markdown output seems like a common use-case for this tool.
Additionally, users would not need to create wrappers around
kubernetes/release
just to customize their output (e.g. release-notes -> JSON -> Go template -> Desired output).Which issue(s) this PR fixes:
Partially addresses urfave/cli#965
Special notes for your reviewer:
This idea was originally suggested by @saschagrunert.
Does this PR introduce a user-facing change?: