-
Notifications
You must be signed in to change notification settings - Fork 569
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
chore(scripts): fix stable release promote script #13204
chore(scripts): fix stable release promote script #13204
Conversation
4bd38cd
to
40651dd
Compare
40651dd
to
cb9f2d1
Compare
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.
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 when CODER=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 do gh auth login
. Just realized that won’t work though, but we could update lib.sh
to fetch the token via gh 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 between GH_
and GITHUB_
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 think GH_
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.com 2>/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 in GITHUB_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.
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 in GITHUB_TOKEN
may be stale, so I opted to refresh this even though it's set.
@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. |
This fixes the promote command to use the right payload...
Switches from GH_ to GITHUB_TOKEN as well and adds gh_auth for completeness.