-
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
Conversation
A new generated diff is ready to view.
A new doc preview is ready to view. |
```markdown | ||
--- | ||
# Adding `aws-sdk-rust` here duplicates this entry into the SDK changelog. | ||
applies_to: ["client", "server", "aws-sdk-rust"] |
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.
breaking: false | ||
# This replaces "tada": | ||
new_feature: false | ||
bug_fix: false |
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.
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 comment
The 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:
- Add an ordering field that takes a RFC-3339 date. Have a CLI that creates new changelog entries and automatically fills this in.
- Have some kind of prominence enum and field to categorize items as more prominent than others.
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.
Great write-up! Thanks for going into a lot of details. It might also be worth mentioning (either under a separate subsection under Implementation
or directly under Safety requirements
) how back-porting from the release branch to main should work with the proposal in the RFC. That naturally brings up the next question, which is when this RFC is implemented, should the implementation be merged to main
or to the release branch?
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Is the transition from Markdown to TOML.
meant to be the transition from Markdown to YAML.
?
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.
Fixed in 0d712a2
authors: ["author1", "author2"] | ||
references: ["smithy-rs#1234", "aws-sdk-rust#1234"] |
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.
These two have always seemed to me unnecessary toil. We could teach the changelogger tool to fill these out by setting authors
to the set of authors that authored the commit that added the changelog entry, and setting references
to any smithy-rs issues and PRs that are mentioned in the commit (which coincides with the GitHub PR description as per our GitHub settings when squash-merging a 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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporated in 0d712a2
I think the first paragraph under the Implementation section covers this, although it doesn't specifically call out the merge from release to main. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.