- Notifications
You must be signed in to change notification settings - Fork928
chore(scripts): fix stable release promote script#13204
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4bd38cd to40651ddCompare40651dd tocb9f2d1CompareThere 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.
Glad to see you found the issue. Nice!
Just giving my ack here, should await devs' review.
| @@ -4,6 +4,9 @@ set -euo pipefail | |||
| # shellcheck source=scripts/lib.sh | |||
| source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" | |||
| # Make sure GITHUB_TOKEN is set for the release command. | |||
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 only works whenCODER=true i.e. inside a Coder workspaces.
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 suggest adding an extra "echo" to indicate 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.
That’s the expected use-case. And if it isn’t set outside a workspace it’ll print a notice to dogh auth login. Just realized that won’t work though, but we could updatelib.sh to fetch the token viagh auth token in this case.
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.
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.
Perhaps we also shouldn't override GITHUB_TOKEN if it's set? And maybe support GH_TOKEN too (in lib.sh)? Thoughts@matifali?
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 as far it works and make it more robust. Let's do this. I don't fully understand the difference betweenGH_ andGITHUB_ prefix for tokens.
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 thinkGH_ is legacy, sometimes still used.
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.
| @@ -4,6 +4,9 @@ set -euo pipefail | |||
| # shellcheck source=scripts/lib.sh | |||
| source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" | |||
| # Make sure GITHUB_TOKEN is set for the release command. | |||
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 suggest adding an extra "echo" to indicate it?
scripts/lib.sh Outdated
| @@ -144,6 +144,8 @@ gh_auth() { | |||
| GITHUB_TOKEN=$(coder external-auth access-token github) | |||
| export GITHUB_TOKEN | |||
| fi | |||
| elif token="$(gh auth token --hostname github.com2>/dev/null)";then | |||
| export GITHUB_TOKEN=$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.
👍🏼
| if [[ -z ${GITHUB_TOKEN:-} ]]; then | ||
| if [[ -n ${GH_TOKEN:-} ]]; then |
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 should always get the new token. The token stored inGITHUB_TOKEN may be stale, so I opted to refresh this even though it's set.
coder external-auth access-token github handles the auto-refreshing automatically.
matifali left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
When inside a coder workspace, We should always get the new token. The token stored inGITHUB_TOKEN may be stale, so I opted to refresh this even though it's set.
mafredri commentedMay 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@matifali Why would there be a token stored in |
If someone sets the token manually or inject via |
@matifali I'm happy you are over-thinking, wouldn't want to break any use-cases. I think the best solution would be to probe the validity of the token, and if it's expired run the path within the if-statement. I think we can leave that for future enhancement, though. |
@mafredri, if this resolves all the script issues we encountered, can should merge soon? Colin and I ran through the scripts for patching mainline and would like to do the same for stable tomorrow. |
f66d044 intomainUh oh!
There was an error while loading.Please reload this page.
This fixes the promote command to use the right payload...
Switches from GH_ to GITHUB_TOKEN as well and adds gh_auth for completeness.