fix(ci): remove Go and refactor install nfpm line #3077
Conversation
| RUN VERSION="$(curl -fsSL https://storage.googleapis.com/kubernetes-release/release/stable.txt)" && \ | ||
| curl -fsSL "https://storage.googleapis.com/kubernetes-release/release/$VERSION/bin/linux/amd64/kubectl" > /usr/local/bin/kubectl \ | ||
| && chmod +x /usr/local/bin/kubectl | ||
| RUN curl -fsSL https://github.com/koalaman/shellcheck/releases/download/v0.7.1/shellcheck-v0.7.1.linux.$(uname -m).tar.xz \ |
There was a problem hiding this comment.
I think there's a Debian package for shellcheck, so you can apt-get install shellcheck here? The current stable release is a little old though: https://packages.debian.org/buster/shellcheck unless you also enable backports: https://packages.debian.org/buster-backports/shellcheck
No worries though, this is fine as it is :)
There was a problem hiding this comment.
We now actually grab shellcheck from npm now, which uses a script that directly downloads/installs it to node_modules - see https://github.com/cdr/code-server/blob/main/package.json#L71 - I landed this in the CI rewrite to make building easier
There was a problem hiding this comment.
nice! so we can remove the curl line?
I was confused that shellcheck in package.json is v1 while the latest release in GitHub is 0.7.1, and it seems that the npm package just downloads the latest release? This would make the build less reproducible, since shellcheck can can change (and break) without any changes on our part
There was a problem hiding this comment.
Oh huh yeah good point. I think we wanted to use the npm version to reduce the number of dependencies users have to manually install so maybe we can fork it, make the version static, and publish @coder/shellcheck or something.
There was a problem hiding this comment.
Could we convince the shellcheck people to release versioned releases that correspond with the upstream versions? e.g. [email protected] or something (upstream version 0.71, first version of the npm package) - that would be useful for other people too and means we don't have to maintain our own thing
There was a problem hiding this comment.
Maybe we can do the same for nfpm.
There was a problem hiding this comment.
That would definitely be better if we could convince upstream! Seems the maintainer is open to proposals: gunar/shellcheck#24 (comment)
There was a problem hiding this comment.
My main concern was that since 1.0.0 is already published it seems a bit sketchy.
| apt-get update && apt-get install -y yarn | ||
| RUN curl -fsSL https://dl.yarnpkg.com/debian/pubkey.gpg | apt-key add - \ | ||
| && echo "deb https://dl.yarnpkg.com/debian/ stable main" | tee /etc/apt/sources.list.d/yarn.list \ | ||
| && apt-get update && apt-get install -y yarn | ||
|
|
||
| # Installs VS Code build deps. |
There was a problem hiding this comment.
In general I think we should condense these RUN commands, each one creates a separate copy-on-write layer and this can lead to "wasted" space. Not something to do now, but just something to keep in mind for the future :)
When you install packages or run an update, apt creates cache files in /var/cache/apt and some other places (it's downloaded packages and the package list), and these consume space in the container image unless you remove them in the same layer (using apt-get clean). There's a magic hook in the Debian docker image that does this automatically, but it's usually better to be explicit (update, install, and clean all in the same step) rather than rely on that magic.
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/
https://dev.to/cloudx/analyzing-the-docker-layers-with-dive-5e7o
| && rm -R shellcheck* | ||
|
|
||
| # Using GitHub release URL is better because now only rely on one service instead of GitHub and install.goreleaser.com | ||
| RUN curl -sSL https://github.com/goreleaser/nfpm/releases/download/v2.3.1/nfpm_2.3.1_Linux_x86_64.tar.gz | tar -C /tmp -zxv nfpm |
There was a problem hiding this comment.
We might want to use the same architecture logic we used for fetching the go binary.
| RUN curl -sSL https://github.com/goreleaser/nfpm/releases/download/v2.3.1/nfpm_2.3.1_Linux_x86_64.tar.gz | tar -C /tmp -zxv nfpm | ||
|
|
||
| RUN VERSION="$(curl -fsSL https://storage.googleapis.com/kubernetes-release/release/stable.txt)" \ | ||
| && curl -fsSL "https://storage.googleapis.com/kubernetes-release/release/$VERSION/bin/linux/amd64/kubectl" >/usr/local/bin/kubectl \ |
There was a problem hiding this comment.
This wasn't introduced by your PR but we should use the detected architecture here as well.
Oh! I didn't even realize it but you're right. I don't use it for development. Not sure if others do, but I would be in favor of closing this PR and removing Not sure if |
|
Closing in favor of #3134 |
Follow-up PR from #3076 (comment)