Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'alpine:3.6' #55

bdlamprecht · 2018-02-26T22:44:19Z

This is an update to both ansible-silo-base and ansible-silo.
I was having really weird issues getting the --bundle to work with certain python libraries as were described in issues #53 and #54.
Some minor enhancements that I made were the following:

Due to the complexity of the command for installing the created bundle, several instructional lines were added to the build script so the user could easily copy / paste after the script ran.
Similar to 1 above, I added the same type of lines to the Makefile for ansible-silo.
Added a few additional libraries that I thought would be worthwhile for others such as bcrypt and napalm (for managing networking devices). That last requirement necessitated leaving pkgconf, libxml2, and libxslt around to connect to Juniper devices via junos-eznc. The additional space for this in ansible-silo-base is only an additional 29% (155MB for 2.0.1 and 201MB for 3.0.0). This size could be reduced further, but didn't want to spend time optimizing for diminishing returns unless it was a real issue.
Added the option to install custom python libraries in the bundle if there was a requirements.txt file in the
Probably something else, but don't recall at the moment.

This builds and runs just fine for me, but as you know Alpine Linux likes to not keep their libraries around for very long once a new version is released, so time is of the essence to get this merged if you approve of the changes.

…pine:3.6'

…he file `playbooks/requirements.txt` exists

bdlamprecht · 2018-02-28T15:56:35Z

@udondan It looks like there might be a problem with your .travis.yml file when trying to install bats (0.4.0-1.1). Not sure how I can help resolve this issue.

udondan

so time is of the essence to get this merged if you approve of the changes.

Yeaaaah, so... that didn't work out very well. :)

I looked at the PR when you sent it but because of the shuffling of all the packages in the base Dockerfile it was really really hard for me to see through. Then forgot to about it. Sorry.

I am about to work on a 3.0 release soonish. Before that I would like to complete this one to not completely make this PR unmergable and give you credit.

I'm happy to switch to the python-alpine based image and adding some commonly used python modules. But I added a couple of comments for other changes.

It looks like there might be a problem with your .travis.yml

Yeah, I got to look into that.

udondan · 2018-06-06T07:29:51Z

Dockerfile


 # Give the user a custom shell prompt
    echo 'export PS1="[ansible-silo $SILO_VERSION|\w]\\$ "' > /home/user/.bashrc &&\

+# Add alias for 'ls -l'


I'd prefer to not have this. Instead we should provide a way for the user to mount a custom profile so any alias, function or what not can be set up as required. Something for another PR I can take care of.

udondan · 2018-06-06T07:35:21Z

Dockerfile


 # Create directory for storing ssh ControlPath
-    mkdir -p /home/user/.ssh/tmp &&\
+RUN mkdir -p /home/user/.ssh/tmp &&\


I believe moving the lines around and splitting into separate layers was beneficial during development/experimenting to speed up builds. Is there a need for this to be merged? The number of layers should be as small as possible.

udondan · 2018-06-06T07:38:45Z

Makefile

+	@echo "  docker run --interactive --tty --rm --volume \"${HOME}/bin:/silo_install_path\" ${SILO_IMG}:${SILO_VERSION} --install"
+	@echo ""
+	@echo "To install '${SILO_IMG}:${SILO_VERSION}' for all users, run the following command:"
+	@echo "  docker run --interactive --tty --rm --volume \"/usr/local/bin:/silo_install_path\" ${SILO_IMG}:${SILO_VERSION} --install"


udondan · 2018-06-06T07:40:25Z

Makefile


 ansible-silo: validate-version
 	@docker build --build-arg "v=$(SILO_VERSION)" --tag "${SILO_IMG}:$(SILO_VERSION)" .
+	@echo ""
+	@echo "To install '${SILO_IMG}:${SILO_VERSION}' for just your user, run the following command:"
+	@echo "  docker run --interactive --tty --rm --volume \"${HOME}/bin:/silo_install_path\" ${SILO_IMG}:${SILO_VERSION} --install"


${HOME}

Should be

\$${HOME}

so the output would be

docker run --interactive --tty --rm --volume "${HOME}/bin:/silo_install_path" grpn/ansible-silo:3.0.0 --install

This way the user can easily copy and share the command.

udondan · 2018-06-06T07:42:43Z

silo/bundle/build

+  pip install -r requirements.txt || exit
+  cd ..
+fi
+


This is installing python modules on the host.

Shouldn't this be added to the Dockerfile of the bundle instead?

udondan · 2018-06-06T07:56:14Z

base.Dockerfile

                enum34==1.1.6\
-                idna==2.5\
-                ipaddress==1.0.18\
-                ncclient==0.5.3\


ncclient Is missing now, which is used by many networking modules. #31

https://github.com/ansible/ansible/search?q=ncclient&unscoped_q=ncclient

udondan · 2018-06-06T08:00:03Z

base.Dockerfile

                pycparser==2.18\
-                pycrypto==2.6.1\


pycrypto is missing.

udondan · 2018-06-06T08:02:19Z

base.Dockerfile

@@ -127,7 +127,6 @@ RUN echo "@testing http://dl-4.alpinelinux.org/alpine/edge/testing" >> /etc/apk/
                       isl\
                       libgomp\
                       libatomic\
-                       pkgconf\


If I understand correct this package is required, because you have junos-eznc in your bundles pip requirements and installing that requires pkgconf. Is that correct? Wouldn't it make more sense then to install it per the bundles Dockerfile?

udondan · 2018-06-06T08:14:56Z

base.Dockerfile

-                       sshpass=1.06-r0 &&\
-
-# Install some required python modules which need compiling
-    apk add --no-cache gcc=6.3.0-r4\


Please keep this block as is. Ideally this apk command would create a virtual package, which later in the Dockerfile can be deleted. This is just not implemented as it was not working together with version pinning at the time of writing. See gliderlabs/docker-alpine#205

udondan · 2018-06-06T08:18:10Z

base.Dockerfile

+RUN echo "@testing http://dl-4.alpinelinux.org/alpine/edge/testing" >> /etc/apk/repositories
+
+# Install common libraries
+RUN apk add --no-cache libssl1.0==1.0.2n-r0\


Please restore original grouping. This makes it really hard to review. Some packages have been removed, some have been added, required version upgrades - but everything has been shuffled so it is really hard to see what actually happened and for what reason.

bdlamprecht · 2018-06-06T13:43:51Z

Yeah, I don't really care about getting credit, I just had to make the changes I did to get the silo working for my project. I can resolve all of the issues which you brought up, but since it:

Has been so long and most likely all of the packages have once again been changed
Is your project which I just "commandeered" for my use

and

You know the intricacies of what you had working more so than me

Why don't you go ahead and make the changes you'd like.
Let me know when I can cancel this PR.
Also, I'd be happy to help you test your version of the ansible-silo 3.0.0 release.

Does that work for you?

udondan · 2018-06-08T14:29:11Z

Hi Brady,

sure, I'll take care of that. I'll make sure to switch to the python-alpine image and add the packages of your PR.

Thanks,
Daniel

Brady Lamprecht added 2 commits February 26, 2018 15:02

Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'al…

32da6b6

…pine:3.6'

Added section to install python libraries specific to the bundle if t…

68fd400

…he file `playbooks/requirements.txt` exists

udondan requested changes Jun 6, 2018

View reviewed changes

This was referenced Oct 23, 2018

adds installation instructions after build #72

Merged

bundle build: shows installation instructions #73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'alpine:3.6' #55

Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'alpine:3.6' #55

bdlamprecht commented Feb 26, 2018

bdlamprecht commented Feb 28, 2018

udondan left a comment

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

udondan Jun 6, 2018

bdlamprecht commented Jun 6, 2018

udondan commented Jun 8, 2018

Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'alpine:3.6' #55

Are you sure you want to change the base?

Updating to base off of 'python:2.7.14-alpine3.6' instead of just 'alpine:3.6' #55

Conversation

bdlamprecht commented Feb 26, 2018

bdlamprecht commented Feb 28, 2018

udondan left a 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

Choose a reason for hiding this comment

bdlamprecht commented Jun 6, 2018

udondan commented Jun 8, 2018