Skip to content
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

CHANGELOG: Store in repo instead of git tag #3056

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

andreabolognani
Copy link
Contributor

@andreabolognani andreabolognani commented Nov 7, 2023

Previously discussed here and here.

Marking as draft because I haven't actually tested any of this O:-)

Also, before it can be merged, someone should populate the CHANGELOG/ directory with release notes for past versions. I can take care of that if the approach proposed here is considered acceptable.

@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 7, 2023
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea and thanks for moving it forward. I added some comments.

releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
releng/release-tool/release-tool.go Outdated Show resolved Hide resolved
@0xFelix
Copy link
Member

0xFelix commented Nov 8, 2023

Looks good from what I can tell, how can we test it?

@andreabolognani
Copy link
Contributor Author

@acardace was the last one to tag a release, so perhaps they can take a look and see if anything looks out of place? If things looks reasonable, I'll start working on the KubeVirt PR that will put all historical release notes into CHANGELOG/ and mark this one as ready.

@andreabolognani
Copy link
Contributor Author

On second thought. While the release notes produced by the script are Markdown-adjacent, they're not actually valid Markdown that renders sensibly on GitHub. Let me try to improve that situation first.

It's still okay to take a look at the git part of the flow though, that's probably not going to change much.

@andreabolognani
Copy link
Contributor Author

I'm also pretty sure I broke all the tests O:-)

@andreabolognani
Copy link
Contributor Author

Big update.

I've fixed the test suite, and also tested things locally to ensure that the process still works - it does, at least as far as I can tell.

The output format is significantly overhauled with the aim of looking good in GitHub.

This is how the recent v1.1.0 release would have looked like in the new format:

The output of 'git shortlog' always ends with a newline, so
calling strings.Split("\n") on it will result in a stray empty
string finding its way into the output. We also skip the entry
for kubevirt-bot late, right before we would print it.

As a consequence, we've always printed out a number of
contributors that's higher (by exactly two) than the number of
items in the contributors list.

Filter out the list early instead, avoiding the problem.

An empty line is added to the top of the "Additional Resources"
text to compensate for the one that is no longer printed at the
end of the contributors list.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Improve the best-effort filter so that it catches more mistakes.

Recent PRs that ended up in the release notes but shouldn't
have, and would have been caught by the improved filter:

  kubevirt/kubevirt#10173
  kubevirt/kubevirt#10370

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Things could go wrong when creating the release notes file or
the git tag, and we shouldn't silently ignore such failures.

We have to skip generation of release notes in the test suite
because of this change, since otherwise we will get error

  unable to generate release notes because no previous tag detected

as there is no real git repository in the mocked environment.
We were already skipping this part in practice, it's just that
until now we also generated an unreported error as part of the
attempt.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The tool is supposed to be largely project-agnostic. In reality,
it's currently only used for KubeVirt, as evidenced by the fact
that the name of the project is hardcoded in at least one place.

Since we're going to use the project's name in more places in
the future, get a bit closer to the intended scenario by asking
for it to be provided as a command line argument.

As an exception, detect the project name automatically for the
main KubeVirt repository. This makes it possible to keep calling
the tool without providing the extra argument.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
@andreabolognani andreabolognani force-pushed the release-tool branch 2 times, most recently from 9ecd623 to 69daba2 Compare November 13, 2023 10:17
@andreabolognani
Copy link
Contributor Author

Added a couple of fixes for existing incorrect behaviors.

Made the code that parses the output of git shortlog more robust, since I've seen at least a couple of variations in the wild. The CHANGELOG/ directory is now created if it doesn't exist, which is good both to remove the dependency on kubevirt/kubevirt#10716 and for projects that might want to start using the tool in the future.

I don't think there's any outstanding concern, so I'm going to mark this as ready now.

How to validate the changes: run

$ make build && ./release-tool --org kubevirt --repo kubevirt --git-user 'Your Name' --git-email your@email --github-token-file /your/token --new-release v1.1.1

then check the /tmp/release-tool/kubevirt/https-kubevirt directory.

@andreabolognani andreabolognani marked this pull request as ready for review November 13, 2023 10:20
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2023
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2023
@andreabolognani
Copy link
Contributor Author

Ping.

Is anything missing? I'd like to get this and kubevirt/kubevirt#10716 merged.

@0xFelix
Copy link
Member

0xFelix commented Dec 5, 2023

Ping @dhiller @xpivarc @acardace

What do you think?

Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@xpivarc @acardace WDYT?

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2023
@xpivarc
Copy link
Member

xpivarc commented Dec 6, 2023

I will bring this up with maintainers. Please hold for now.

@xpivarc
Copy link
Member

xpivarc commented Dec 11, 2023

/cc @aburdenthehand
how does this look for you?

@aburdenthehand
Copy link
Contributor

Looks really great. Thanks @andreabolognani for pushing forward on this.

One question: Will this only be generated at the same time when we release a version? Currently we get the pre-release-tagged log ahead of the release which can be modified to categorise the release notes and also used for the user-guide release notes PR.
(I'm happy as long as there is a way to generate this info ahead of the final release).

@andreabolognani
Copy link
Contributor Author

One question: Will this only be generated at the same time when we release a version? Currently we get the pre-release-tagged log ahead of the release which can be modified to categorise the release notes and also used for the user-guide release notes PR.
(I'm happy as long as there is a way to generate this info ahead of the final release).

The process shouldn't change much: right now the release notes are embedded in the tag the moment it's created, after my changes they will be added to the repository in a commit that is created right before the tag.

AFAICT currently the only way to generate the release notes ahead of the actual release is to use the --dry-run option, which will cause the tool to skip the git push step, and then inspect the temporary repository. That will still work. Or is there another way that I've missed?

I've now looked at the user-guide repository and I've seen that there is some automated processing going on there. That part would obviously have to be adapted. And the task of categorizing entries as changes, bug fixes etc., that's manual, right? If so, I'm not sure where this step would fit into the existing, automated release workflow.

Maybe we could start asking people to explicitly categorize their PRs at creation time? Something like

```release-note:deprecation
Feature foo has been deprecated.
```

In any case, I wonder if it still makes sense to have the the release notes as part of user-guide once they're browsable via GitHub. Maybe we can just yank that section?

Sort of unrelated, but I've just realized that the way I've implemented things doesn't really work too well when it comes to tagging alphas, betas and rcs. I need to go back and revisit that part. I'll move the PR back to draft for the moment.

@andreabolognani andreabolognani marked this pull request as draft December 12, 2023 13:33
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2023
@aburdenthehand
Copy link
Contributor

aburdenthehand commented Dec 13, 2023

AFAICT currently the only way to generate the release notes ahead of the actual release is to use the --dry-run option, which will cause the tool to skip the git push step, and then inspect the temporary repository. That will still work. Or is there another way that I've missed?

I'm not sure what the mechanism is but when we prepare the RC I have been able to edit the tag text through github, which has given me two weeks to manually organise.
But running a dry-run locally for me works for now. As I understand it, it wouldn't change the actual release changelog that this script will prepare but means I can prepare the user-guide.

Re: Why also publish the user guide - Last year when I was wondering the same thing I ran a bunch of findability tests comparing the two and the search is much better for the user guide than in github. I think it's also useful when searching a term in the user guide to have a related release note returned in the search results.

I've now looked at the user-guide repository and I've seen that there is some automated processing going on there.
That part would obviously have to be adapted. And the task of categorizing entries as changes, bug fixes etc., that's manual, right? If so, I'm not sure where this step would fit into the existing, automated release workflow.

Maybe we could start asking people to explicitly categorize their PRs at creation time? Something like

Feature foo has been deprecated.

We have github labels already defined to handle all the categories (sig: and kind:). I imagine using something like 'release-note:deprecation' would be useful for kubevirt-bot to automatically add a deprecation label but tbh it might just be better to encourage greater label awareness/usage. Especially with the upcoming changes to SIG groups that has been talked about this year.

With ~consistent labels, I imagine we can add a filtering mechanism to the great work you've done in this PR to automatically handle the categorisation of the release notes into their relevant sections. That would be a dream.

(And the user-guide script you mention has been a bit broken since we went to v1.0, so it's already a manual process. I'm not too bothered by it, but eventually I presume we can build it into the automation of the release process as well)

@andreabolognani
Copy link
Contributor Author

I'm not sure what the mechanism is but when we prepare the RC I have been able to edit the tag text through github

Tags can't be edited directly through GitHub, much as commits can't. So I assume you're talking about the release notes attached to the tag, e.g. https://github.com/kubevirt/kubevirt/releases/tag/v1.1.0. If you compare that to the output of git show v1.1.0, you'll see that the latter is missing all the nice categorization.

which has given me two weeks to manually organise.

So is it you taking care of sorting all changes into the appropriate categories for every release? Can you please describe the workflow in some detail?

But running a dry-run locally for me works for now. As I understand it, it wouldn't change the actual release changelog that this script will prepare but means I can prepare the user-guide.

IIUC you base your work on the release notes generated for RC, right? If so, you wouldn't even need do do any dry-runs: just update your local clone once the RC has been pushed and grab the release notes from the CHANGELOG/ directory.

Re: Why also publish the user guide - Last year when I was wondering the same thing I ran a bunch of findability tests comparing the two and the search is much better for the user guide than in github. I think it's also useful when searching a term in the user guide to have a related release note returned in the search results.

I have no strong objection to publishing the same information in multiple places, but I would like it to be consistent. If for whatever reason the release notes can't be the same everywhere, the version in the repo should IMO be the "best" one, as it's the only one that's cryptographically signed (via git tags) and guaranteed to survive events such as changes in hosting.

We have github labels already defined to handle all the categories (sig: and kind:). I imagine using something like 'release-note:deprecation' would be useful for kubevirt-bot to automatically add a deprecation label but tbh it might just be better to encourage greater label awareness/usage. Especially with the upcoming changes to SIG groups that has been talked about this year.

With ~consistent labels, I imagine we can add a filtering mechanism to the great work you've done in this PR to automatically handle the categorisation of the release notes into their relevant sections. That would be a dream.

I really don't think I can justify spending a lot more time branching further into KubeVirt release tooling than I already have :) but since the labels matching each of the release notes categories appear to already exist, I think it would absolutely make a lot of sense to enforce via the usual PR tooling that each PR that contains release notes also has an appropriate label associated to it before being merged.

Once that guarantee is in place, it should be fairly simple to update the tool further so that items are automatically sorted into the various buckets.

@aburdenthehand
Copy link
Contributor

Tags can't be edited directly through GitHub, much as commits can't. So I assume you're talking about the release notes attached to the tag, e.g. https://github.com/kubevirt/kubevirt/releases/tag/v1.1.0. If you compare that to the output of git show v1.1.0, you'll see that the latter is missing all the nice categorization.

Yes, sorry, that's what I'm referring to.

So is it you taking care of sorting all changes into the appropriate categories for every release? Can you please describe the workflow in some detail?

Once the tag is created I can edit the markdown and go through the release notes one by one and sort them (usually locally so I can work on a copy and compare against an original). Those with the kind/, sig/ or bug/ labels are easy; for the rest, some are easy to deduce and the remaining I reach out to a higher authority.

IIUC you base your work on the release notes generated for RC, right? If so, you wouldn't even need do do any dry-runs: just update your local clone once the RC has been pushed and grab the release notes from the CHANGELOG/ directory.

Even better :)

I have no strong objection to publishing the same information in multiple places, but I would like it to be consistent. If for whatever reason the release notes can't be the same everywhere, the version in the repo should IMO be the "best" one, as it's the only one that's cryptographically signed (via git tags) and guaranteed to survive events such as changes in hosting.

+1

I really don't think I can justify spending a lot more time branching further into KubeVirt release tooling than I already have :) but since the labels matching each of the release notes categories appear to already exist, I think it would absolutely make a lot of sense to enforce via the usual PR tooling that each PR that contains release notes also has an appropriate label associated to it before being merged.

Oh yes, absolutely. I didn't mean to imply that the filtering should be included in this PR. I was just describing an existing alternative to having a release note type in the field itself.
I very much appreciate the work you've already put into this and don't think it should be blocked by any additional ideas. As you say, these can be added as an additional update.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Mar 8, 2024

@andreabolognani: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-prow-kubevirt-labels-update-precheck ce0ddd3 link true /test pull-prow-kubevirt-labels-update-precheck
pull-prow-nmstate-labels-update-precheck ce0ddd3 link true /test pull-prow-nmstate-labels-update-precheck

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-sigs/prow repository. I understand the commands that are listed here.

@kubevirt-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@kubevirt-bot kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 26, 2024
@kubevirt-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

@kubevirt-bot kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 25, 2024
@kubevirt-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

@kubevirt-bot
Copy link
Contributor

@kubevirt-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

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-sigs/prow repository.

@xpivarc
Copy link
Member

xpivarc commented Oct 25, 2024

/remove-lifecycle rotten
/reopen

@kubevirt-bot kubevirt-bot reopened this Oct 25, 2024
@kubevirt-bot
Copy link
Contributor

@xpivarc: Reopened this PR.

In response to this:

/remove-lifecycle rotten
/reopen

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-sigs/prow repository.

@kubevirt-bot kubevirt-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lgtm Indicates that a PR is ready to be merged. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants