-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
CI: modify release-notes.py to auto-categorize pull requests based on labels #4850
Conversation
Signed-off-by: Ansh Goyal <[email protected]>
scripts/release-notes.py
Outdated
categories = { | ||
'CI': 'changelog:ci', | ||
'Feature': 'changelog:feature', | ||
} |
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.
We can modify this object as we want the result to be. For example:
categories = {
'## Changes related to CI': 'changelog:ci',
'## New Features': 'changelog:feature',
'## Breaking changes ⚠️': 'changelog:breaking-change',
}
this would categorise the PRs and would make sure the other person can just copy-paste the console logs
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, we need to do that, and keep the order as in the current CHLG
Codecov ReportAll modified lines are covered by tests ✅ 📢 Thoughts on this report? Let us know!. |
lgtm, but one main comment: we already have a template for release notes in the CHANGELOG, so we want to match the output to that template. That means :
What happens when we have commits directly to a branch without a PR? |
Signed-off-by: Ansh Goyal <[email protected]>
Done. Since Python v3.7, insertion order matters in objects, I have kept the order as it was in previous Changelogs.
They all would go to the Others category |
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
We can test the changes more practically, by assigning changelog labels to some of the previously merged PRs after the previous release |
Signed-off-by: Ansh Goyal <[email protected]>
scripts/release-notes.py
Outdated
{'title': '#### 🚧 Experimental Features', 'label': 'changelog:exprimental'}, | ||
{'title': '#### CI Improvements', 'label': 'changelog:ci'}, | ||
{'title': '', 'label': 'changelog:test'}, | ||
{'title': '', 'label': 'changelog:skip'} |
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.
'title': None
would be more idiomatic Python.
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.
Makes sense. I am writing Python code for the first time :)
scripts/release-notes.py
Outdated
for category, prefix in categories.items(): | ||
if label.startswith(prefix): | ||
return category | ||
return 'Other' # Default category if no matching prefix is found |
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.
return 'Other' # Default category if no matching prefix is found | |
return 'Uncategorized' # Default category if no matching prefix is found |
scripts/release-notes.py
Outdated
commit_labels = get_pull_request_labels(token, args.repo, pull_id) | ||
changelog_labels = [label for label in commit_labels if label.startswith('changelog:')] | ||
|
||
category = 'Other' |
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.
maybe define a constant at the top
UNCATTEGORIZED = 'uncategorized'
scripts/release-notes.py
Outdated
@@ -88,17 +108,70 @@ def main(token, repo, num_commits, exclude_dependabot): | |||
if not pulls: | |||
short_sha = sha[:7] | |||
commit_url = commit['html_url'] | |||
print(f'* {msg} ([@{author}]({author_url}) in [{short_sha}]({commit_url}))') | |||
# Check if the commit has changelog label | |||
commit_labels = get_pull_request_labels(token, args.repo, pull_id) |
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 there are not pulls
where are you getting pull_id
?
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.
Correct actually. Now, I just threw all commits without pull_ids to the UNCATEGORISED
group.
scripts/release-notes.py
Outdated
for result in other_results: | ||
print(result) | ||
|
||
if skipped_dependabot: |
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.
should this be indented left?
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.
Missed that. Thanks
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
scripts/release-notes.py
Outdated
@@ -66,6 +83,10 @@ def main(token, repo, num_commits, exclude_dependabot): | |||
print('Retrieved', len(commits), 'commits') | |||
|
|||
# Load PR for each commit and print summary | |||
print("Processing...") |
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 needed
scripts/release-notes.py
Outdated
# Print categorized pull requests | ||
for category, results in category_results.items(): | ||
if results and category: | ||
print(f'{category}:') |
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.
new line after header
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 need an empty line after the header? Currently it is like:
Header 1:
PRs Table
Header 2:
PRs Table
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.
we do, look at the existing changelog format. In plain text markdown it's much easier to see headers when they are spaced, not jammed with text
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
scripts/release-notes.py
Outdated
|
||
if skipped_dependabot: | ||
print(f"\n(Skipped {skipped_dependabot} dependabot commit{'' if skipped_dependabot == 1 else 's'})") | ||
if not commits_with_multiple_labels: |
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.
odd coupling of the logic, better to have each section to be self-contain, e.g. by always calling print() at the end
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.
Done 🚀
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
## Which problem is this PR solving? - related to: #4850 ## Description of the changes - This PR adds a Dynamic Loading Bar Functionality to `release-notes.py` - Knowing exactly what percentage our script is completed processing at each second provides a better UX ## How was this change tested? - Locally ![npaefr0x](https://github.com/jaegertracing/jaeger/assets/94157520/e956a443-f5a3-4162-bec0-9adce9dd43bf) ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
Which problem is this PR solving?
Description of the changes
categories
object could be modified to accommodate more categories along with some meaningful titles likeChanges related to CI: changelog:ci
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test