fix: update install script for alpine #3707

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

Jump to bottom

Merged

jsjoeio merged 2 commits into main from jsjoeio-fix-alpine-install-script

Jul 2, 2021

Merged

fix: update install script for alpine #3707

jsjoeio merged 2 commits into main from jsjoeio-fix-alpine-install-script

Jul 2, 2021

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Jul 1, 2021 •

edited

Loading

This PR fixes the install script to work on Alpine 64-bit. Our arch function doesn't recognize 32-bit Alpine, which fallsback to npm/yarn install (which is why it works on 32-bit Alpine), but tries to use the standalone install for 64-bit, which doesn't work.

This fixes that.

Fixes #3421

Also, thank you to @b-trav for filing an issue about the broken links in install.sh!

@jsjoeio


 fix: modify install script for alpine

5a6af17

@jsjoeio jsjoeio added the bug label

Jul 1, 2021

@jsjoeio jsjoeio self-assigned this

Jul 1, 2021

@jsjoeio

Copy link

Contributor Author

jsjoeio commented Jul 1, 2021

I don't have access to Alpine 64-bit. @code-asher can you test? (Or is there a way for me to get access to some VM running Alpine 64-bit?

@jsjoeio jsjoeio marked this pull request as ready for review

July 1, 2021 23:41

@jsjoeio jsjoeio requested a review from a team as a code owner

July 1, 2021 23:41

jawnsy

jawnsy previously approved these changes

Jul 2, 2021

View reviewed changes

Copy link

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

jawnsy

jawnsy reviewed

Jul 2, 2021

View reviewed changes

install.sh Outdated

return

if [ -f /etc/alpine-release ]; then

Copy link

@jawnsy jawnsy Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too big of a change for this, I just wanted to mention that there's a standard file that contains distro info, os-release, which also seems to work on Alpine:

15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it fedora cat /etc/os-release | grep "^ID="
ID=fedora
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it ubuntu cat /etc/os-release | grep "^ID="
ID=ubuntu
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it debian cat /etc/os-release | grep "^ID="
ID=debian
15:17 coder@jawnsy-m ~/projects/m $ docker run --rm -it alpine cat /etc/os-release | grep "^ID="
ID=alpine

Here's the info about it: https://www.freedesktop.org/software/systemd/man/os-release.html

There's also a doc link earlier in the file here, do we want to use the new docs site instead? https://github.com/cdr/code-server/blob/5a6af177256056e316d0056b291fb3a8159c5776/install.sh#L70

Copy link

Member

@code-asher code-asher Jul 2, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo we use that a few lines up actually. @jsjoeio let's expand the case statement to include alpine instead of checking /etc/alpine-release. Actually I'm not sure why we need the case at all. Let's just return the ID as-is.

Copy link

Member

@code-asher code-asher Jul 2, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I misread. We only have the case for ID_LIKE then we fall back to ID. So this should already be returning alpine which means no changes are necessary in distro (can remove the /etc/alpine-release block).

Copy link

Contributor Author

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a doc link earlier in the file here, do we want to use the new docs site instead?

Good catch! I'll update those.

Copy link

Contributor Author

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to mention that there's a standard file that contains distro info, os-release, which also seems to work on Alpine

TIL that's a thing! Thanks for the tip @jawnsy 🙌

So this should already be returning alpine which means no changes are necessary in distro (can remove the /etc/alpine-release block).

Ah, didn't realize that. Sweet! Less code to add. Removed.

@jsjoeio jsjoeio marked this pull request as draft

July 2, 2021 17:16

@jsjoeio jsjoeio requested review from jawnsy and code-asher

July 2, 2021 17:22

@jsjoeio jsjoeio marked this pull request as ready for review

July 2, 2021 17:22

@jsjoeio jsjoeio dismissed jawnsy’s stale review

July 2, 2021 17:22

Made the requested changes!

jawnsy

jawnsy approved these changes

Jul 2, 2021

View reviewed changes

install.sh

echoerr "Please install npm or yarn to install code-server!"

echoerr "You will need at least node v12 and a few C dependencies."

echoerr "See the docs https://github.com/cdr/code-server/blob/v3.10.2/docs/install.md#yarn-npm"

echoerr "See the docs https://coder.com/docs/code-server/v3.10.2/install#yarn-npm"

Copy link

@jawnsy jawnsy Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to bump the version number each release, or should we link to /latest/ here?

Copy link

Contributor Author

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, I think we should replace latest with the version number in this case actually so that old docs will link to the correct branch.

I think @bpmct and @BrunoQuaresma thought we should use the version (source).

We do have a script called release-prep.sh which handles updating these links for us. See here

Copy link

Contributor

@BrunoQuaresma BrunoQuaresma Jul 2, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we want to keep the version so the script release-prep can handle the update for us.

@codecov

Copy link

codecov bot commented Jul 2, 2021 •

edited

Loading

Codecov Report

Merging #3707 (686ecd8) into main (1ff09b8) will not change coverage.
The diff coverage is n/a.

❗ Current head 686ecd8 differs from pull request most recent head 908b2c8. Consider uploading reports for the commit 908b2c8 to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@
## main #3707 +/- ##
=======================================
 Coverage 61.55% 61.55% 
=======================================
 Files 35 35 
 Lines 1813 1813 
 Branches 365 365 
=======================================
 Hits 1116 1116 
 Misses 588 588 
 Partials 109 109

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ff09b8...908b2c8. Read the comment docs.

@jsjoeio


 fix: use new docs links in install script

908b2c8

@jsjoeio jsjoeio enabled auto-merge

July 2, 2021 17:29

@jsjoeio jsjoeio force-pushed the jsjoeio-fix-alpine-install-script branch from 686ecd8 to 908b2c8 Compare

July 2, 2021 17:29

@jsjoeio jsjoeio added the docs label

Jul 2, 2021

@jsjoeio jsjoeio linked an issue

Jul 2, 2021

that may be closed by this pull request

Link to install.sh returns a 404 #3710

Closed

code-asher

code-asher reviewed

Jul 2, 2021

View reviewed changes

install.sh

# code-server's automatic install script.

# See https://github.com/cdr/code-server/blob/main/docs/install.md

# See https://github.com/cdr/code-server/blob/main/docs/install

Copy link

Member

@code-asher code-asher Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also go to coder.com? Without .md it seems to 404.

Copy link

Contributor Author

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang it. I did a serach for .md and must have accidentally done a search and replace. I'll fix.

Copy link

Contributor Author

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3711

@jsjoeio jsjoeio merged commit f8a1a1c into main

Jul 2, 2021

@jsjoeio jsjoeio deleted the jsjoeio-fix-alpine-install-script branch

July 2, 2021 17:45

@jsjoeio jsjoeio mentioned this pull request

Jul 2, 2021

chore: fix broken link in install.sh #3711

Merged

Labels

bug docs

4 participants

@jsjoeio @jawnsy @BrunoQuaresma @code-asher

fix: update install script for alpine #3707

fix: update install script for alpine #3707

Uh oh!

Conversation

@jsjoeio jsjoeio commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsjoeio commented Jul 1, 2021

Uh oh!

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@code-asher code-asher Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@jawnsy jawnsy Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@BrunoQuaresma BrunoQuaresma Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@code-asher code-asher Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@jsjoeio jsjoeio commented Jul 1, 2021 •

edited

Loading

@code-asher code-asher Jul 2, 2021 •

edited

Loading

@code-asher code-asher Jul 2, 2021 •

edited

Loading

@BrunoQuaresma BrunoQuaresma Jul 2, 2021 •

edited

Loading

codecov bot commented Jul 2, 2021 •

edited

Loading