-
Notifications
You must be signed in to change notification settings - Fork 193
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 RFC for changing the smithy-rs changelog process #3313
Changes from 1 commit
c1239dc
d4e9683
0d712a2
d650586
50b441b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
RFC: File-per-change changelog | ||
============================== | ||
|
||
> Status: RFC | ||
> | ||
> Applies to: client and server | ||
|
||
For a summarized list of proposed changes, see the [Changes Checklist] section. | ||
|
||
Historically, the smithy-rs and AWS SDK for Rust's changelogs and release notes have been | ||
generated from the `changelogger` tool in `tools/ci-build/changelogger`. This is a tool built | ||
specifically for development and release of smithy-rs, and it requires developers to add | ||
changelog entries to a root `CHANGELOG.next.toml` file. Upon release, the `[[smithy-rs]]` entries | ||
in this file go into the smithy-rs release notes, and the `[[aws-sdk-rust]]` entries are associated | ||
with a smithy-rs release commit hash, and added to the `aws/SDK_CHANGELOG.next.json` for | ||
incorporation into the AWS SDK's changelog when it releases. | ||
|
||
This system has gotten us far, but it has always made merging PRs into main more difficult | ||
since the central `CHANGELOG.next.toml` file is almost always a merge conflict for two PRs | ||
with changelog entries. | ||
|
||
This RFC proposes a new approach to change logging that will remedy the merge conflict issue, | ||
and explains how this can be done without disrupting the current release process. | ||
|
||
The proposed developer experience | ||
--------------------------------- | ||
|
||
There will be a `changelog/` directory in the smithy-rs root where | ||
developers can add changelog entry Markdown files. Any file name can be picked | ||
for these entries. Suggestions are the development branch name for the | ||
change, or the PR number. | ||
|
||
The changelog entry format will change to make it easier to duplicate entries | ||
across both smithy-rs and aws-sdk-rust, a common use-case. | ||
|
||
This new format will make use of Markdown front matter in the YAML format. | ||
This change in format has a couple benefits: | ||
- It's easier to write change entries in Markdown than in a TOML string. | ||
- There's no way to escape special characters (such as quotes) in a TOML string, | ||
so the text that can be a part of the message will be expanded. | ||
|
||
While it would be preferable to use TOML for the front matter (and there are libraries | ||
that support that), it will use YAML so that GitHub's Markdown renderer will recognize it. | ||
|
||
A changelog entry file will look as follows: | ||
|
||
```markdown | ||
--- | ||
# Adding `aws-sdk-rust` here duplicates this entry into the SDK changelog. | ||
applies_to: ["client", "server", "aws-sdk-rust"] | ||
authors: ["author1", "author2"] | ||
references: ["smithy-rs#1234", "aws-sdk-rust#1234"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two have always seemed to me unnecessary toil. We could teach the changelogger tool to fill these out by setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good idea. Perhaps a good follow up item to implement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorporated in 0d712a2 |
||
# The previous `meta` section is broken up into its constituents: | ||
breaking: false | ||
jdisanti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# This replaces "tada": | ||
new_feature: false | ||
bug_fix: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you may want some sort of ordering field if you want any sort of consistent ordering during change-log rendering (but maybe it's fine to just go alphabetically?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we may need to experiment with this to get a good balance between rendered result and dev experience. Two ideas that come to mind to try:
|
||
--- | ||
|
||
Some message for the change. | ||
``` | ||
|
||
Implementation | ||
-------------- | ||
|
||
When a release is performed, the release script will generate the release notes, | ||
update the `CHANGELOG.md` file, copy SDK changelog entries into the SDK, | ||
and delete all the files in `changelog/`. | ||
jdisanti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
### SDK Entries | ||
|
||
The SDK changelog entries currently end up in `aws/SDK_CHANGELOG.next.json`, and each entry | ||
is given `age` and `since_commit` entries. The age is a number that starts at zero, and gets | ||
incremented with every smithy-rs release. When it reaches a hardcoded threshold, that entry | ||
is removed from `aws/SDK_CHANGELOG.next.json`. The SDK release process uses the `since_commit` | ||
to determine which changelog entries go into the next SDK release's changelog. | ||
|
||
The SDK release process doesn't write back to smithy-rs, and history has shown that it | ||
can't since this leads to all sorts of release issues as PRs get merged into smithy-rs | ||
while the release is in progress. Thus, this `age`/`since_commit` dichotomy needs to | ||
stay in place. | ||
|
||
The `aws/SDK_CHANGELOG.next.json` will stay in place in its current format without changes. | ||
Its JSON format is capable of escaping characters in the message string, so it will be | ||
compatible with the transition from Markdown to TOML. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 0d712a2 |
||
|
||
The `SDK_CHANGELOG.next.json` file has had merge conflicts in the past, but this only | ||
happened when the release process wasn't followed correctly. If we're consistent with | ||
our release process, it should never have conflicts. | ||
|
||
### Safety requirements | ||
|
||
Implementation will be tricky since it needs to be done without disrupting the existing | ||
release process. The biggest area of risk is the SDK sync job that generates individual | ||
commits in the aws-sdk-rust repo for each commit in the smithy-rs release. Fortunately, | ||
the `changelogger` is invoked a single time at the very end of that process, and only | ||
the latest `changelogger` version that is included in the build image. Thus, we can safely | ||
refactor the `changelogger` tool so long as the command-line interface for it remains | ||
backwards compatible. (We _could_ change the CLI interface as well, but it will | ||
require synchronizing the smithy-rs changes with changes to the SDK release scripts.) | ||
|
||
At a high level, these requirements must be observed to do this refactor safely: | ||
- The CLI for the `changelogger render` subcommand _MUST_ stay the same, or have minimal | ||
backwards compatible changes made to it. | ||
- The `SDK_CHANGELOG.next.json` format can change, but _MUST_ remain a single JSON file. | ||
If it is changed at all, the existing file _MUST_ be transitioned to the new format, | ||
and a mechanism _MUST_ be in place for making sure it is the correct format after | ||
merging with other PRs. It's probably better to leave this file alone though, or make | ||
any changes to it backwards compatible. | ||
|
||
Changes checklist | ||
----------------- | ||
|
||
- [ ] Refactor changelogger and smithy-rs-tool-common to separate the changelog | ||
serialization format from the internal representation used for rendering and splitting. | ||
- [ ] Implement deserialization for the new Markdown entry format | ||
- [ ] Incorporate new format into the `changelogger render` subcommand | ||
- [ ] Incorporate new format into the `changelogger split` subcommand | ||
- [ ] Port existing `CHANGELOG.next.toml` to individual entries | ||
- [ ] Update `sdk-lints` to fail if `CHANGELOG.next.toml` exists at all to avoid losing | ||
changelog entries during merges. | ||
- [ ] Dry-run test against the smithy-rs release process. | ||
- [ ] Dry-run test against the SDK release process. | ||
|
||
[Changes Checklist]: #changes-checklist |
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 want to take this opportunity to add a
fixes
field?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.
Not a bad idea.