Add git-lfs to the image. #5

pwagland · 2017-05-09T08:28:47Z

Fixes #4.

cptactionhank

Isn't there a less verbose to add GPG keys?

pwagland · 2017-05-09T09:18:23Z

Potentially you could drop the key into a file, and import from the file. But that would add an extra layer into the resulting image.

pwagland · 2017-05-09T11:19:12Z

Let me know if you want that change made, and I'll update the pull request.

pwagland · 2017-05-13T22:19:28Z

I have update the request to split the key out into its own file, and updated the Dockerfile to follow best practices from https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/ . This means that the image ends up slightly smaller, even though git-lfs has been added.

pwagland · 2017-05-15T08:47:57Z

@cptactionhank Is there anything different that you would like to see to get this merged in?

cptactionhank · 2017-05-15T14:24:29Z

Yes, I have concerns about the practice of doing it this way and are examining how some of the other official images handles this eg. postgres, and others.

I'll get back to you

cptactionhank · 2017-05-15T14:45:37Z

Dockerfile

+    && echo "Removing packages that are no longer needed" \
+    && dpkg --purge \
+            apt-transport-https \
+            xmlstarlet \


xmlstarlet is used by the entrypoint script and shall not be purged

cptactionhank · 2017-05-15T14:46:40Z

Dockerfile

 # Install Atlassian Bamboo and helper tools and setup initial home
 # directory structure.
 RUN set -x \
+    && echo 'Adding repository and required key to install git-lfs' \
+    && apt-key add /tmp/github-lfs-repo.gpgkey \
+    && rm /tmp/github-lfs-repo.gpgkey \


Please remove this.

A file layer with the key still exists so the removal doesn't provide much value

cptactionhank · 2017-05-15T15:05:43Z

Dockerfile

@@ -5,13 +5,25 @@ ENV BAMBOO_HOME     /var/atlassian/bamboo
 ENV BAMBOO_INSTALL  /opt/atlassian/bamboo
 ENV BAMBOO_VERSION  6.0.0

+# Copy in the gpg key for github_git-lfs so that we can load git-lfs
+# into the image.
+COPY github-lfs-repo.gpgkey /tmp


Please remove these added lines

If you remove those lines then you don't have the key for the new repository.

cptactionhank · 2017-05-15T15:05:58Z

Dockerfile

 # Install Atlassian Bamboo and helper tools and setup initial home
 # directory structure.
 RUN set -x \
+    && echo 'Adding repository and required key to install git-lfs' \


Please remove this line

cptactionhank · 2017-05-15T15:06:27Z

Dockerfile

 # Install Atlassian Bamboo and helper tools and setup initial home
 # directory structure.
 RUN set -x \
+    && echo 'Adding repository and required key to install git-lfs' \
+    && apt-key add /tmp/github-lfs-repo.gpgkey \


Please remove this (it could also be added using curl and a pipe command)

I don't like just pulling in random files from the net without some verification that I am getting a verified version of it.

The other way to do it that I have seen is to pull in the key from the central key servers, so something like (taken from the sonar-scanner image):

# pub 2048R/D26468DE 2015-05-25 # Key fingerprint = F118 2E81 C792 9289 21DB CAB4 CFCA 4A29 D264 68DE # uid sonarsource_deployer (Sonarsource Deployer) <infra@sonarsource.com> # sub 2048R/06855C1D 2015-05-25 && gpg --keyserver ha.pool.sks-keyservers.net --recv-keys F1182E81C792928921DBCAB4CFCA4A29D26468DE \

I get that and their supplied script does the same thing as you did originally, but I think it is simpler and have less potential maintenance doing it this way. The trusted GPG key is taken from the same place as where the install script is located so this should be just as trusted as well.

But I would prefer the other way you suggested in long term, this is what the official postgres image does as well.

cptactionhank · 2017-05-15T15:08:11Z

github-lfs-repo.gpgkey

+aZeW04kRtFDOPinz0faE8hvsxzsVgkKye1c2vkXKdOXvA3x+pZzlTHtcgMOhjKQA
+sA==
+=H60S
+-----END PGP PUBLIC KEY BLOCK-----


Please remove this whole file

cptactionhank · 2017-05-15T15:10:20Z

Dockerfile

+    && apt-get install --quiet --yes --no-install-recommends \
+               apt-transport-https \
+               xmlstarlet \
+    && echo 'deb http://packagecloud.io/github/git-lfs/debian/ jessie main' > /etc/apt/sources.list.d/github_git-lfs.list \


Please remove this

If this is removed, then git-lfs cannot be (easily) installed. apt-transport-https is needed for the git-lfs repo, as it is only exported over https, via a redirect.

cptactionhank · 2017-05-15T15:13:16Z

Dockerfile

    && apt-get update --quiet \
-    && apt-get install --quiet --yes --no-install-recommends xmlstarlet \
+    && apt-get install --quiet --yes --no-install-recommends \
+               git-lfs \


git-lfs/git-lfs has a docker recipe stated as

RUN build_deps="curl ca-certificates" && \ apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends ${build_deps} && \ curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | bash && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends git-lfs && \ git lfs install && \ DEBIAN_FRONTEND=noninteractive apt-get purge -y --auto-remove ${build_deps} && \ rm -r /var/lib/apt/lists/*

although curl is already available in the openjdk image and ca-certificates is available as well which should boil it down to:

RUN apt-get update && \ curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | bash && \ apt-get install -y --no-install-recommends git-lfs && \ git lfs install && \ rm -r /var/lib/apt/lists/*

Please add the following lines instead

&& curl --silent https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | bash \ && apt-get install --quiet --yes --no-install-recommends git-lfs \ && git lfs install \

If we are to trust their GPG key we might as well trust their install script.

FWIW, that script basically just does the lines that I added, I pulled out the essential elements of that script into this file. But I am happy to just call that shell script if you are.

cptactionhank · 2017-05-15T15:28:00Z

Dockerfile

+    && apt-get update --quiet \
+    && apt-get install --quiet --yes --no-install-recommends \
+               apt-transport-https \
+               xmlstarlet \


I would prefer dependencies in a single line

To be honest, so do I, this is just following the guidelines of the "best practices document". If you are happy with one line, so am I!

pwagland · 2017-05-16T09:37:15Z

Requested changes are in.

We also remove packages that are no longer needed, and properly clean the apt-get cache according to https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/ . The end result, is that we end up with a smaller image, and bamboo supports git-lfs. Fixes cptactionhank#4

cptactionhank · 2017-05-17T21:25:54Z

Acceptance tests works on my machine. Thank you for your work!

cptactionhank reviewed May 9, 2017

View reviewed changes

pwagland force-pushed the master branch from 768db0c to 753968a Compare May 13, 2017 22:12

cptactionhank reviewed May 15, 2017

View reviewed changes

pwagland force-pushed the master branch 3 times, most recently from 65f184e to e388320 Compare May 16, 2017 09:34

pwagland force-pushed the master branch from e388320 to 79b3db4 Compare May 16, 2017 14:29

pwagland force-pushed the master branch from 79b3db4 to c9c931f Compare May 16, 2017 14:30

cptactionhank merged commit 2415c6c into cptactionhank:master May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add git-lfs to the image. #5

Add git-lfs to the image. #5

pwagland commented May 9, 2017

cptactionhank left a comment

pwagland commented May 9, 2017

pwagland commented May 9, 2017

pwagland commented May 13, 2017

pwagland commented May 15, 2017

cptactionhank commented May 15, 2017

cptactionhank May 15, 2017

cptactionhank May 15, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017

cptactionhank May 15, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017 •

edited

Loading

cptactionhank May 17, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017

pwagland May 16, 2017

cptactionhank May 15, 2017

pwagland May 16, 2017

pwagland commented May 16, 2017

cptactionhank commented May 17, 2017

Add git-lfs to the image. #5

Add git-lfs to the image. #5

Conversation

pwagland commented May 9, 2017

cptactionhank left a comment

Choose a reason for hiding this comment

pwagland commented May 9, 2017

pwagland commented May 9, 2017

pwagland commented May 13, 2017

pwagland commented May 15, 2017

cptactionhank commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwagland May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwagland commented May 16, 2017

cptactionhank commented May 17, 2017

pwagland May 16, 2017 •

edited

Loading