-
Notifications
You must be signed in to change notification settings - Fork 729
Add Changelog script #695
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 Changelog script #695
Conversation
Codecov Report
@@ Coverage Diff @@
## master #695 +/- ##
==========================================
+ Coverage 85.03% 85.04% +0.01%
==========================================
Files 35 35
Lines 1156 1157 +1
Branches 172 172
==========================================
+ Hits 983 984 +1
Misses 90 90
Partials 83 83
Continue to review full report at Codecov.
|
|
I noticed that this issue was in the java library as well. If this PR gets accepted I can PR it there as well or you can. Whatever's easiest for you. |
|
@thinkingserious I don't want to be a pain, just don't want this to slip through the cracks. Can I get review on this? |
|
This is awesome @PatOConnor43! I would just say that we should include an option where I can pass our GitHub token in for the API call so that we don't get hit by the rate limiting issue. |
|
I can definitely do that. I'll get that updated tonight. Thanks! |
|
@thinkingserious This should be ready for review again. It supports passing in the token with a -t flag or as an env var. It will also fall back to the un-authenticated way if no token is provided. |
misterdorm
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.
I think this is fine as-is, but I left a few suggestions for improvements if you would like to take those on. Thanks!
generate_changelog.sh
Outdated
| url="$host/$origin/$repo/pull/$pr" | ||
| api_url="$api_host/repos/$origin/$repo/pulls/$pr" | ||
| curl_command="curl -s $api_url" | ||
| if [ "$authorization_token" != "" ] |
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 can do: if [ -z "$authorization_token" ] instead
generate_changelog.sh
Outdated
| curl_command="curl -s $api_url" | ||
| if [ "$authorization_token" != "" ] | ||
| then | ||
| curl_command="curl -H \"Authorization: token $authorization_token\" -s $api_url" |
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 would suggest:
curl_command="$curl_command -H \"Authorization: token $authorization_token\""
So there is a single place to update the curl command if needed in the future.
generate_changelog.sh
Outdated
| -v v_content="$content" \ | ||
| '/All notable changes to this project will be documented in this file/{print;print v_header1;print v_header2;print v_content;next}1' CHANGELOG.md \ | ||
| > CHANGELOG.md.temp | ||
| mv CHANGELOG.md.temp CHANGELOG.md |
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.
mv -f just to be sure?
| # Generate changelog from last tag | ||
| latest_release="$(git describe --tags --abbrev=0)" | ||
| create_changelog | ||
| echo "Successfully wrote to CHANGELOG.md" No newline at end of file |
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 create_changelog fails for some reason, it would be nice to display an error message, instead of "success" regardless of the outcome.
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 added a set -e at the top of the file. I think this should halt the script and print the expected error message. Does that work for you?
|
Can do 👍 I'll get this updated tonight |
|
Thanks for the review @misterdorm. I noticed that this issue was posted in a couple of the sendgrid repos. Would it be helpful for me to make PRs to them as well? I don't want to spam this PR in a bunch of repos if you guys just want to handle it. |
|
@PatOConnor43 If you want to create PRs in the other repos where someone else hasn't already picked up the issue, please feel free. There are at least a few other repos where others have implemented this, and in those cases it would just be redundant. Thanks! |
|
Hello @PatOConnor43, |
|
@thinkingserious Any chance I can still get one of those sweet Sendgrid Hacktoberfest shirts for this PR? |
|
Closing as we're now using internal tooling for this. |
Resolves #693
Checklist
Short description of what this PR does:
generate_changelog.shscript that should take the newest PRs that have not been part of the latest release and creates an entry for them in the CHANGELOG.md file.Example usage and output:
diff
If you have questions, please send an email to SendGrid, or file a GitHub Issue in this repository.