Skip to content
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

Add initial scripts for Ubuntu Cosmic (18.10) #121

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

seemethere
Copy link
Contributor

@seemethere seemethere commented Jul 9, 2018

Cosmic isn't scheduled for GA until October 18, 2018 but let's go ahead and add the scripts to get ahead of the curve.

Biggest change is a shift from btrfs-tools -> libbtrfs-dev, which was already done for Debian Buster here #113

cuttlefish

Signed-off-by: Eli Uriegas eli.uriegas@docker.com

@seemethere seemethere requested review from a team and thaJeztah July 9, 2018 22:17
jose-bigio
jose-bigio previously approved these changes Jul 9, 2018
Copy link
Contributor

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Left some suggestions 😅

@@ -0,0 +1,29 @@
FROM ubuntu:cosmic

RUN apt-get update && apt-get install -y apparmor bash-completion libbtrfs-dev build-essential cmake curl ca-certificates debhelper dh-apparmor dh-systemd git libapparmor-dev libdevmapper-dev libltdl-dev libseccomp-dev pkg-config vim-common libsystemd-dev --no-install-recommends && rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

For maintainability, it would be good to put each package on a new line, and keep them sorted alphabetically. Doing so makes it easier to compare between Dockerfiles (and to do more granular review comments on GitHub);

something like;

RUN apt-get update
	&& apt-get install -y \
		apparmor \
		bash-completion \
		build-essential \
		ca-certificates \
		cmake curl \
		debhelper \
		dh-apparmor \
		dh-systemd \
		git \
		libapparmor-dev \
		libbtrfs-dev \
		libdevmapper-dev \
		libltdl-dev \
		libseccomp-dev \
		libsystemd-dev \
		pkg-config \
		vim-common \
	--no-install-recommends \
	&& rm -rf /var/lib/apt/lists/*

COPY common/ /root/build-deb/debian
COPY build-deb /root/build-deb/build-deb

RUN mkdir -p /go/src/github.com/docker && \
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how important caching is for these Dockerfiles, but this could be done earlier in the Dockerfile.

There's some other lines as well that could be put earlier, to prevent churn / unneeded re-creation of layers

Copy link
Member

Choose a reason for hiding this comment

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

(very) quick attempt to re-order a bit more optimised (but possibly this should be done for all Dockerfiles afterwards (haven't checked);

FROM ubuntu:cosmic

ENV DISTRO ubuntu
ENV SUITE cosmic

ENV GOPATH /go
ENV PATH $PATH:/usr/local/go/bin:$GOPATH/bin
ENV DOCKER_BUILDTAGS apparmor seccomp selinux
ENV RUNC_BUILDTAGS apparmor seccomp selinux

RUN apt-get update
	&& apt-get install -y \
		apparmor \
		bash-completion \
		build-essential \
		ca-certificates \
		cmake curl \
		debhelper \
		dh-apparmor \
		dh-systemd \
		git \
		libapparmor-dev \
		libbtrfs-dev \
		libdevmapper-dev \
		libltdl-dev \
		libseccomp-dev \
		libsystemd-dev \
		pkg-config \
		vim-common \
	--no-install-recommends \
	&& rm -rf /var/lib/apt/lists/*

RUN mkdir -p /go/src/github.com/docker && \
	mkdir -p /go/src/github.com/opencontainers && \
	ln -snf /engine /root/build-deb/engine && \
	ln -snf /cli /root/build-deb/cli && \
	ln -snf /root/build-deb/engine /go/src/github.com/docker/docker && \
	ln -snf /root/build-deb/cli /go/src/github.com/docker/cli

WORKDIR /root/build-deb
ENTRYPOINT ["/root/build-deb/build-deb"]

ARG GO_VERSION

RUN curl -fSL "https://golang.org/dl/go${GO_VERSION}.linux-arm64.tar.gz" | tar xzC /usr/local

COPY common/ /root/build-deb/debian
COPY build-deb /root/build-deb/build-deb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dockerfiles have been re-optimized! And I removed the requirement for vim-common, it wasn't actually a requirement

jose-bigio
jose-bigio previously approved these changes Jul 20, 2018
Copy link
Contributor

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

LGTM

WORKDIR /root/build-deb

ARG GO_VERSION
RUN curl -fSL "https://golang.org/dl/go${GO_VERSION}.linux-amd64.tar.gz" | tar xzC /usr/local
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only line that differs among each of the 4 Dockerfiles?
Would it be desirable to have an arch tag as a build arg, and then pass the arch tag in here in a similar way to how the GO_VERSION is being passed in?

That way we don't need to check in and maintain 4 distinct Dockerfiles?

This would probably require a bigger refactor so that the other Dockerfiles for other ubuntu distributions worked in the same way. As a result, if this change is desirable we can track it somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the only line that differs and yes it'd require a pretty decent refactor. Let's leave that for another PR.

jose-bigio
jose-bigio previously approved these changes Oct 22, 2018
Copy link
Contributor

@jose-bigio jose-bigio left a comment

Choose a reason for hiding this comment

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

LGTM

@jose-bigio
Copy link
Contributor

Will give an LGTM on green 😱

Signed-off-by: Eli Uriegas <eli.uriegas@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants