-
Notifications
You must be signed in to change notification settings - Fork 103
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
Generate release notes automatically from pull requests #3174
Generate release notes automatically from pull requests #3174
Conversation
This is inspired by GitHub's auto-generated notes, but we are creating our own to make it more powerful in the future.
This will be useful for backporting the changes for 4.0 Note 4.0.559.0 includes all of the relational layer commits, which is way too much for it to fetch that PR so many times, so don't run with that version.
This makes it easier to clear cached entries after adding missing labels
This depends on a placeholder that I have not yet added...
I am going to regenerate these notes automatically, and then fill in anything that was lost. Committing this separately makes it easier for me to test.
this will make patch releases go by the original version they were built on
This determines where releases should go relative to the minor version, end previous version. I switched it to update the lines field one note at a time, because I had trouble reasoning about the behavior otherwise and getting it right.
When the heading is collapsed, it confuses sphinx, so use <h4> instead.
This makes it more conformant with the markdown spec
Rather than just putting the url, make it a link, with a title. This also helps, because sphinx doesn't auto-detect links
This makes the username clickable, and turns the PR link into a proper markdown link.
If you do 4.0.559.6 to 4.0.561.0, you don't want to pick up any of the commits in 4.0.559.6, just the ones in 4.0.561.0. This isn't a problem when there's linear history, but could be potentially problematic with patch releases, although probably not in the flow that would actually happen in practice.
I tried changing my flow to generate all the non-patch releases and then the patch releases, and it put the patches in the wrong places
for category in label_config['categories']: | ||
for label in category['labels']: | ||
if label in label_names: | ||
return category['title'] |
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 believe this means that if we tag a PR multiple times, it only appears once (in the first label category in the config that it matches)
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, that is the intent. I think one later thing we could do that would be pretty clean would be to insert other labels (from the set of categories) next to the PR description. so you might have something like:
New Features
build improvement I added a feature
But that might just be too noisy, and really, PRs should be focused, and having multiple labels where secondary labels actually matter is probably a bad sign.
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 the main one I worry about is "breaking change". Most breaking changes will probably be also either features or bug fixes. In the past, with the hand-rolled release notes, I've done stuff like mention the fix/feature in the appropriate section, and then specified what the "breaking" part is in the breaking change. So you might have something like:
Breaking changes
- Continuations for streaming aggregate queries are no longer compatible with versions below 4.1.8.0 - PR #x
Features
- Streaming aggregate query continuations do not lose work if they are stopped by an out of band limit - PR #x
You can structure that as a single thing, but it means that someone looking at the breaking change may only see the bug fix/feature and have to then do more digging to see if they care about the way it is breaking
Two conflicts: 1. main updated CONTRIBUTING.md to reference docs/sphinx/ReleaseNotes.md, in a section I replaced, so I had to update the new section to reference the new ReleaseNotes location 2. Changes to release_build-publish/action.yml to support sphinx conflicted with moving the release notes to before the build, and the new steps.get_new_version.
This was originally just copied from GitHub releases' approach, but consensus on the team was that we should focus on the change, not who did it.
I thought I saw this being optional, but it fails without it.
This seems to work locally, but fail in github actions... I don't know why, and didn't investigate, because using a raw string is the right thing to do anyways.
I tried to just fetch 1000 commits, but it still had trouble finding the tag for the previous release. Testing, this only took about 5 seconds to checkout the repo, and like 3 with a fetch-depth of 1.
This commit is from incrementing the version, and is not in github, so we will fail when trying to find associated prs.
This is essential for testing this on other repositories
Also including versions 4.0.561.0 4.0.562.0 4.0.564.0 4.0.565.0 4.0.566.0 4.0.567.0 4.0.575.0 4.1.4.0 4.1.6.0
Also including versions 4.0.559.1 4.0.559.3 4.0.559.4
This adds two things: 1. Add sub-bullet points to the PR that introduced the 4.0.559 patch branch, along with backporting a few other changes 2. Add back the notes we had written up for the 4.1 release.
shell: bash | ||
run: git push origin HEAD "${{ steps.get_new_version.outputs.version }}" |
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'm still thinking about this, because it seems wrong to me that we'd have a documented release with release notes and everything but if the gradle build below fails, the release may not exist. This seems worse than the situation before where we'd potentially have a release that was published (in that there could be Jars available for it to download), but for which there were no notes, in which case it was kind of on the consumer for attempting to use something that is undocumented. (The unpublished release notes would also be left as is, so anything that went into the release-without-notes would just be included in the next release, which is accurate enough.)
I sort of wonder if we should just push once at the very end. We were hesitant to do this before because of conflicts, but I think we can actually make the system a bit more resilient to that by having it pull before merging and create a PR if it can't cleanly merge, and then continuing. (One thing we'd have to check: if we merge before pushing, and therefore don't have a linear history, does the compare
set include any PRs that are in a sibling branch of the tag?) We could also move the testing to a separate phase of the build, which could speed things up and also give us the mixed-mode information at the right time, and it would improve the logs of the currently hard-to-read "build and publish" step.
I don't want to keep litigating this forever, though, and we should be able to start with what's in the PR. I think one consequence is that we may need to do some clean up if the wrong thing happens, during the build or publishing stages, but we can live with that, at least for now. (This probably means regenerating the notes. Like, if version x
exists, version x + 1
has notes for it but is not published or not published completely, we may need to create notes for version x + 2
with x
as the basis and delete x + 1
from the notes.)
|
||
#### Mixed Mode Test Results |
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.
Did you mean to delete the 4.1.8.0 mixed mode results in this push?
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 did not, I will fix that...
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 guess, I should say that I meant to delete it, but I also meant to add it back, which I forgot to do.
This mostly LGTM. I left a few comments about style, but the only thing that seems "blocking" is making sure we don't lose the mixed-mode results for 4.1.8.0. There's a bigger discussion about when the least erroneous time to push is, but I also think we can take this to unblock the new release notes process. |
Co-authored-by: Alec Grieser <[email protected]>
This alleviates two issues:
I considered just using GitHub's
generate-notes
functionality, but that is rather limited. This change doesn't really expand on using that yet, but puts us in a better place to do so.This also updates the release workflow to use the new script, and updates the release notes so that they have the new format for every version since
4.0.559.0
. This also means the release notes now include the patch releases of4.0.559.*
. I also did some manual additions to the release notes.