Skip to content

fix(ci): remove Go and refactor install nfpm line #3077

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix(ci): remove Go and refactor install nfpm line #3077

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29192c5
fix(ci): remove Go
jsjoeio Apr 8, 2021
0b2bd14
fix(ci): only rely on GitHub for nfpm
jsjoeio Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 16 additions & 24 deletions ci/images/debian10/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ RUN apt-get update
RUN apt-get install -y curl gnupg

# Installs node.
RUN curl -fsSL https://deb.nodesource.com/setup_12.x | bash - && \
apt-get install -y nodejs
RUN curl -fsSL https://deb.nodesource.com/setup_12.x | bash - \
&& apt-get install -y nodejs

# Installs 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
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

RUN apt-get install -y build-essential
Expand All @@ -24,25 +24,17 @@ RUN apt-get install -y gettext-base
RUN apt-get install -y git rsync unzip jq

# Installs shellcheck.
RUN curl -fsSL https://github.com/koalaman/shellcheck/releases/download/v0.7.1/shellcheck-v0.7.1.linux.$(uname -m).tar.xz | \
tar -xJ && \
mv shellcheck*/shellcheck /usr/local/bin && \
rm -R shellcheck*

# Install Go.
RUN ARCH="$(uname -m | sed 's/x86_64/amd64/; s/aarch64/arm64/')" && \
curl -fsSL "https://dl.google.com/go/go1.14.3.linux-$ARCH.tar.gz" | tar -C /usr/local -xz
ENV GOPATH=/gopath
# Ensures running this image as another user works.
RUN mkdir -p $GOPATH && chmod -R 777 $GOPATH
ENV PATH=/usr/local/go/bin:$GOPATH/bin:$PATH

# More stable than go get
RUN curl -sfL https://install.goreleaser.com/github.com/goreleaser/nfpm.sh | sh

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 \
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can do the same for nfpm.

Copy link
Member

Choose a reason for hiding this comment

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

That would definitely be better if we could convince upstream! Seems the maintainer is open to proposals: gunar/shellcheck#24 (comment)

Copy link
Member

@code-asher code-asher Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My main concern was that since 1.0.0 is already published it seems a bit sketchy.

| tar -xJ \
&& mv shellcheck*/shellcheck /usr/local/bin \
&& 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
Copy link
Member

Choose a reason for hiding this comment

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

We might want to use the same architecture logic we used for fetching the go binary.


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 \
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't introduced by your PR but we should use the detected architecture here as well.

&& chmod +x /usr/local/bin/kubectl
RUN curl -fsSL https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash
RUN helm plugin install https://github.com/instrumenta/helm-kubeval

Expand Down