-
Notifications
You must be signed in to change notification settings - Fork 13
Add changelog management tooling and backfill existing release history #45
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
base: develop
Are you sure you want to change the base?
Conversation
- new-entry.py allows for creating entried for a specific package, type, and a description. - new-release.py consolidates new entries into a versioned json file for preservation. - render.py will generate a CHANGELOG.md file using the versioned json files and a jinja2 template.
…transcribe-streaming
…sagemaker-runtime-http2
ubaskota
left a comment
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.
Overall looks good. I've added some comments. Please, let me know if you have any questions.
| # Get package .changes directory and ensure it exists | ||
| changes_dir = get_package_changes_dir(package_name) | ||
| changes_dir.mkdir(exist_ok=True) | ||
|
|
||
| # Create next-release directory for pending changes | ||
| next_release_dir = changes_dir / "next-release" | ||
| next_release_dir.mkdir(exist_ok=True) |
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.
Both create_change_entry and create_summary_entry functions repeat the same logic for directory setup. Do you think they can be consolidated into a single function?
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.
+1; I'm not sure I understand the reason for duplicating this logic. It's minor, but this could easily be one function:
def ensure_changes_dir(package_name):
changes_dir = PROJECT_ROOT_DIR / "clients" / package_name / ".changes"
changes_dir.mkdir(parents=True, exist_ok=True)
return changes_dir
| ) -> str: | ||
| """Create or update the release summary for the next release.""" | ||
| # Get package .changes directory and ensure it exists | ||
| changes_dir = get_package_changes_dir(package_name) |
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.
Will there be a case where the package_name is invalid? Do we need to add a validation?
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.
If it doesn't exist, we'll get the following error:
FileNotFoundError: [Errno 2] No such file or directory: '/Users/gytndd/dev/GitHub/aws-sdk-python/clients/aws-sdk-bedrock-runtimes/.changes'
I can add a better error message
| versions.append(file.stem) | ||
|
|
||
| # Sort by semantic version (oldest first) | ||
| versions.sort(key=lambda v: [int(x) for x in v.split(".")]) |
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.
Nit: Would using versions.sort(key=.., reverse=True) be better than reversing the sorted list inside render_changes?
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.
Either one works tbh. I'll add a reverse parameter to get_sorted_versions and provide True when we call it
| # Generate unique filename | ||
| timestamp = int(time.time() * 1_000_000) | ||
| unique_id = uuid.uuid4().hex | ||
| filename = f"{package_name}-{change_type}-{timestamp}-{unique_id}.json" |
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.
For filename, timestamp is already in microseconds. Wouldn't that be enough? What is the benefit of adding uuid to it?
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.
Relying on timestamps alone will never really guarantee uniqueness since our infra will be creating changelog entries in rapid succession (and in parallel in the future). See the example below:
>>> timestamps = []
>>> for i in range(1000):
... timestamp = int(time.time() * 1_000_000)
... if timestamp in timestamps:
... print("Found duplicate")
... else:
... timestamps.append(timestamp)
...
Found duplicate
Found duplicate
Found duplicateWe could increase the precision, however, using uuid is more robust. I decided to include the timestamp in attempt to achieve natural ordering of changelog entries when rendering them out. However, that isn't really working right now and I'll handle it in a separate PR. It's more of a nice to have.
I'm gonna remove timestamp for now and only use uuid.
| }, | ||
| { | ||
| "type": "dependency", | ||
| "description": "**Updated**: `smithy_aws_core[eventstream, json]` from `~=0.2.0` to `~=0.3.0`." |
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.
This seems like an odd thing to bold to begin with, and it is less readable (for me) in both JSON and markdown.
Not a huge thing, and we shouldn't change them retroactively, but if we are going to start using markdown syntax in changelog entries, we should agree on guidelines for when and how to apply them and record them in our contributing.md file.
| def get_package_changes_dir(package_name: str) -> Path: | ||
| package_path = PROJECT_ROOT_DIR / "clients" / package_name | ||
| changes_dir = package_path / ".changes" | ||
| return changes_dir |
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.
Nit:
Personally I don't find this any more readable than a single line function of return PROJECT_ROOT_DIR / "clients" / package_name / ".changes".
If you do or prefer this for any reason, keep it as is.
| # Get package .changes directory and ensure it exists | ||
| changes_dir = get_package_changes_dir(package_name) | ||
| changes_dir.mkdir(exist_ok=True) | ||
|
|
||
| # Create next-release directory for pending changes | ||
| next_release_dir = changes_dir / "next-release" | ||
| next_release_dir.mkdir(exist_ok=True) |
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.
+1; I'm not sure I understand the reason for duplicating this logic. It's minor, but this could easily be one function:
def ensure_changes_dir(package_name):
changes_dir = PROJECT_ROOT_DIR / "clients" / package_name / ".changes"
changes_dir.mkdir(parents=True, exist_ok=True)
return changes_dir
| parser.add_argument( | ||
| "-t", | ||
| "--type", | ||
| # TODO: Remove the 'breaking' option once this project is stable. |
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.
Just a thought- maybe we leave this in permanently? For minor "breaks" that we will continue to do that offer more benefit than pain to customers.
One example that may be upcoming in boto is switching to using standard instead of legacy retry mode.
| version_data["summary"] = summary | ||
|
|
||
| with open(version_file, "w") as f: | ||
| json.dump(version_data, f, indent=2) |
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.
| json.dump(version_data, f, indent=2) | |
| json.dump(version_data, f, indent=2) | |
| f.write("\n") |
Adding a newline at the end of the file will prevent git from showing the warning and help with tools that prefer files end in newlines.
| entry_file.unlink() | ||
| removed_count += 1 | ||
| except OSError as e: | ||
| print(f"Warning: Could not remove {entry_file}: {e}", file=sys.stderr) |
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.
The intention here is to ignore the error quietly and continue? Do you think it's possible that this error will get ignored and leave behind the entry file and duplicate the entry for the next release?
Note
These scripts are adapted from smithy-lang/smithy-python#552, with the addition of support for release summary entries (
SUMMARY.json).Summary
aws-sdk-bedrock-runtime,aws-sdk-sagemaker-runtime-http2,aws-sdk-transcribe-streaming).Changelog Tooling
This PR introduces three scripts under
scripts/changelog/:new-entry.py.changes/next-release/new-release.py0.3.0.json)render.pyCHANGELOG.mdfrom version JSON files using a Jinja2 templateWorkflow
new-entry.py --package <name> --type <type> --description "..."new-release.pyconsolidates entries into<version>.jsonrender.pyregeneratesCHANGELOG.mdfrom all version filesTest plan
new-entry.pycreates entries in the correct locationnew-release.pyconsolidates entries and cleans upnext-release/render.pygenerates accurateCHANGELOG.mdoutputBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.