Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Release Best Practices Guide #149
base: main
Are you sure you want to change the base?
Release Best Practices Guide #149
Changes from all commits
d9c3457
04ce4ec
e5beb8e
243d920
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@riverma Adding a picture(s) of how to this step generates release notes would be helpful for new users wanting to adopt SLIM.
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 suggestion @pogi7 - will do here 👍
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.
Could use a brief hint here on how to get a token—maybe down on the FAQ?
In retrospect, I think this is awfully risky. Your token could afford a lot of permissions unless it's carefully scoped—and putting it into a file makes it too easy to check into version control (yay detect-secrets!) Maybe it should be an environment variable?
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.
My comment is to link to the GH Actions documentation directly -- I wouldn't put "" here, but instead I would make it explicit with
${{ secrets.GITHUB_TOKEN }}
. Then in2.2
, something like, "Create a release notes template (.yml
) that's authorized to publish your project."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 that's OK because "secrets.GITHUB_TOKEN" is mnemonically explicit.
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.
@nutjob4life - I was debating between an environment variable and a file. I felt that a file could be kept secure on a system with permission control, whereas the environment variable approach keeps a credential on the command-line history. Though I see the risk of committing this file and your point! Are these our only two choices, I'm curious about best practices here.
@jpl-jengelke - agreed to linking to GH directly and letting their docs handle the UI specifics.
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.
OK, I like the concept of building release notes based off the GH API and the script looks light enough. When I first read about this I though of using GH's built-in facilities to just copy it. Could it all be done in an action file without a separate Python component? What if the actual release notes stored in the 'releases' section of the repository are used or the macros that create them there are reused? I think if GH products are reused then GH handles maintenance, the notes are uniform and the maintenance footprint is reduced. (I can see Python versions and module releases being pesky as time goes forward, plus shouldn't there be a dependencies file (requirements.txt)?
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 script should probably include some comments at the top about dependencies. For example, it does
import requests
so needs a Python installation (or virtual environment) with Requests installed. Bonus points for mentioning a version number ofrequests
too 👍 (same fortqdm
,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.
Echoes my comment above... Also, Python 3+.
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.
Also may want to add a little try-except error catching to explain to a casual user what's wrong. What if the template itself is munged up? That can be hard to catch.
Some minimal logging (even just print statements) may be helpful, too.
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 could use Tenacity here to automate retries over the function. It would also handle the manifest other potential exceptions that could occur.
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 move the
retry_num += 1
up and add anif retry_num < max_retries
to condition the sleep? The "edge" case is that GitHub gives non-200 every single time, but we end up doing a final sleep for 2 × 2⁴ + [0.0..1.0] seconds—up to 33 seconds—to then just abort without a final attempt.In other words, change from
attempt…sleep…attempt…sleep…attempt…sleep…attempt…sleep…abort
→attempt…sleep…attempt…sleep…attempt…sleep…attempt…abort
🤷Not a huge deal but I've been known to be rather pedantic 😂
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.
Let me look into this thanks @nutjob4life
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.
See comment above about putting the token into the
.yml
(or.yaml
where I come from 😁) file.