- Notifications
You must be signed in to change notification settings - Fork928
feat(install.sh): add support for--mainline (default) and--stable#12858
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
feat(install.sh): add support for--mainline (default) and--stable#12858
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f68b2b5 to339acb9Compare2d79f59 toace59dfCompare| # separated numbers and without utilizing the sort -V flag for the | ||
| # best compatibility. | ||
| curl -fsSL https://api.github.com/repos/coder/coder/releases | | ||
| awk -F'"' '/"tag_name"/ {print $4}' | |
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.
nit: you didn't want to usejq? :)
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’d love to, but it’s a dependency we can’t afford.
| @@ -106,9 +118,18 @@ echo_standalone_postinstall() { | |||
| return | |||
| fi | |||
| channel=mainline | |||
| advisory="To install our stable release (v${STABLE_VERSION}), use the --stable flag. " | |||
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.
nit: the trailing space might not be most convenient form, but I see why you have added this way, one condition less.
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 initially had the entire advisory as two separate vars, then I noticed the repetition and extracted common parts 😅, I can go back to that method if you think it’s preferable.
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.
Probably not worth it, this is just a single condition :) Thanks for sharing more context!
install.sh Outdated
| @@ -282,8 +303,12 @@ main() { | |||
| --version=*) | |||
| VERSION="$(parse_arg "$@")" | |||
| ;; | |||
| --edge) | |||
| EDGE=1 | |||
| # Support edge for backwards compatibility. | |||
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.
nit: isn't itbackward (no -s)?
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.
Both forms seem to be used, but perhaps “backward” is the more common one.
| # https://gist.github.com/lukechilds/a83e1d7127b78fef38c2914c4ececc3c#gistcomment-2758860 | ||
| version="$(curl -fsSLI -o /dev/null -w "%{url_effective}" https://github.com/coder/coder/releases/latest)" | ||
| version="${version#https://github.com/coder/coder/releases/tag/v}" | ||
| echo "${version}" |
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.
might be confusing with VERSION
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 the upper/lower case is a good enough distinction, and this practice (case) is often used to separate script global and function local variables. Although inherently case doesn’t differentiate between scope, for that local/typeset/declare needs to be used. Since we’re supporting posix shell though, we don’t try to use such “advance features”. 😄
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.
Alright, we can always adoptlocal keyword :)
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.
Perfect!!!
| cath <<EOF | ||
| Coder has been installed to | ||
| Coder ${channel} release v${VERSION} installed. ${advisory}See our releases documentation or GitHub for more information on versioning. |
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.
Awesome 👏
Uh oh!
There was an error while loading.Please reload this page.
This PR adds support for installing either the mainline or stable version of Coder.
The
--edgeflag was deprecated and--mainline(default) and--stableadded in it's place.The previous implementation for "edge" did not take into account that the release order might not be reliable, so the implementation was updated to perform version-based sorting on all the tags available in the JSON response for releases, and the highest version numer is selected.
In the future we may want to filter out releases on a certain criteria (e.g. exclude draft/prerelease), however, I decided to ommit this to avoid premature feature development and to keep the change simpler.
Note: I decided against embedding
CODER_MAINLINE_VERSIONin the script because this introduces added complexity in the release flow:CODER_MAINLINE_VERSIONand the GitHub Actions build completing, before build completion, the new version is not available, even if cutFixes#12461