Ability to override ubuntu:18.04 #527

matyasmarkovics · 2019-03-13T10:25:55Z

My objective was to be able to build based off of debian:stretch. Given the similarities of this was just a matter of introducing a build-arg to the Dockerfile.

I've fixed some minor issues as well, while I trying to run tests.

I hope you'll find these improvements helpful.

…image

kingdonb · 2019-03-13T12:10:17Z

Thanks for your neat contribution!

A lot of these changes seem like they will hamper usability of the image. I'm not sure we can merge this as-is, but don't let that deter you – I like the idea of making the upstream base pluggable. I'd also like to preserve the capability of users with only docker and no make binary to easily build the image, without reading the Makefile. (The BUILD_ARG FROM makes that harder to discover.)

Some of the other changes you've contributed mostly without comment, I'm wondering why they are needed. Again don't let any critical feedback deter you. I appreciate the effort you've put in and thanks for sharing! I'll come back to this and see if I can suggest changes that might make it less confusing, say for example adding some documentation about the BUILD_ARG.

We could have used that strategy for multiarch support, which is another feature that has been explored but hasn't been merged, mostly because it makes things more complicated. I'm not sure I'm ready to say "let's not use a BUILD_ARG", but it maybe needs some more thought put into it.

kingdonb · 2019-03-31T17:07:12Z

I found another project using build ARGs and I think I'm making this out to be harder than it needs

https://github.com/ms-ati/docker-rvm

In the example I link above, the arg is specified like:

ARG RVM_RUBY_VERSIONS="2.3.7 2.5.1 ruby-head"
FROM msati/docker-rvm

Would you consider adjusting your PR so that it specifies the default version in the Dockerfile, like docker-rvm does here, in their README? Then you could also revert your change to the build: task in the Makefile, and still preserve the capability of the person building to simply run docker build image/ without any build_arg. On second thought, the Makefile does not really need to change back, but perhaps it would be better to not have to let this be specified in both places.

I don't know about polymorphic tasks in Make, maybe there's a way you can still infer this from an environment variable. I just want to preserve the capability to build on Windows, with nothing but a Docker engine. The operator should not need to run "make build" or discover a build_arg to get the default behavior. (And changing the default in this case would definitely be a Major Version bump)

edit: I am not sure I've correctly understood the purpose of the ARG keyword here... sorry

matyasmarkovics · 2019-03-31T19:19:51Z

Sure, I can define a default image in the Dockerfile. The reason I went with the Makefile approach was the same reason you've mentioned, to not have it duplicated. - Admittedly I missed the use case of just running pure docker. Wrt. the build target, perhaps the best is to have -ifdef clause in the Makefile and only supply the --build-arg flag if some environment variable is set.

matyasmarkovics · 2019-03-31T19:41:07Z

image/cleanup.sh

@@ -9,7 +9,7 @@ rm -rf /tmp/* /var/tmp/*
 rm -rf /var/lib/apt/lists/*

 # clean up python bytecode
-find / -name *.pyc -delete


I remember getting plenty of "permission denied" errors here as find was trying to traverse into /proc and /sysfolders.

matyasmarkovics · 2019-03-31T19:46:19Z

Makefile

 		if test "$$ID" = ""; then echo "Container is not running."; exit 1; fi && \
-		IP=$$(docker inspect $$ID | grep IPAddr | sed 's/.*: "//; s/".*//') && \


Probably docker version dependent, but the sed substitution wasn't giving the expected output, so I've found out how the do the same with docker itself.

matyasmarkovics · 2019-03-31T20:07:57Z

test/runner.sh

@@ -29,14 +27,14 @@ fi
 trap cleanup EXIT

 echo " --> Enabling SSH in the container"
-docker exec -t -i $ID /etc/my_init.d/00_regen_ssh_host_keys.sh -f


Neither of these commands need input, so in that sense they don't need to be interactive nor should they expect their input to be a terminal. I wouldn't bother, but I was getting an error on this lines, suggesting to remove at least one of -i or '-t'. I've googled around and found a pretty good explanation: https://stackoverflow.com/a/54254380 and realised both could be removed.

matyasmarkovics · 2019-03-31T20:28:01Z

test/runner.sh

 sleep 1 # Give container some more time to start up.
-ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i /tmp/insecure_key -p $SSHPORT [email protected] \


I could not connect using localhost, only with the containers' IP. I wanted to remove duplication between these lines and the ssh target in Makefile. Apparently, I've "reverted" someone's contribution though: 2f0e1ad#diff-2c0426df66e885ae0431057d1c516e62

I'm actually more interested in line 38 and 39, what effect will result when insecure_key isn't copied into the container, or if it will have any effect on the tests.

It looks like there is a new error raised during the make test:

Warning: Permanently added '[127.0.0.1]:32768' (ECDSA) to the list of known hosts. mesg: ttyname failed: Inappropriate ioctl for device

This is the only strange thing I found that looks wrong. I wonder if it's related to removing -t -i setting.

matyasmarkovics · 2019-03-31T20:36:26Z

I've added comments to this PR for the changes not related to the BASE_IMAGE arg. The common background of these is that I've built and tested the project within a docker container and some of the scripts had to ironed out to make that work. Might be an edge-case, but I thought these improvements valuable regardless.

…-build-arg to build target if BASE_IMAGE is defined, document the use of BASE_IMAGE in README

…container via SSH, fix SSH connection by IP issue on Mac-OS

kingdonb · 2019-04-13T14:09:54Z

Thank you for the updates! I have some other stuff to do first this weekend, then I intend to swing back around and test this.

It seems like a big enough change to warrant multiple reviews. I appreciate you marking up the changes that I didn't understand with explanations. If the default behavior of the build hasn't changed now that Ubuntu is specified as the default arg for BASE_IMAGE, then I think this can be merged, if another contributor agrees.

Thanks for adding documentation about BASE_IMAGE and NAME as well!

kingdonb

It's a little disturbing that when I start the build using:

$ make build BASE_IMAGE=debian:stretch NAME=kingdonb/stretch

I'm greeted with

docker build -t kingdonb/stretch:0.11 --build-arg BASE_IMAGE=debian:stretch --rm image
Sending build context to Docker daemon  64.51kB
Step 1/7 : ARG BASE_IMAGE=ubuntu:18.04
Step 2/7 : FROM $BASE_IMAGE
stretch: Pulling from library/debian
e79bb959ec00: Pull complete
Digest: sha256:724b0fbbda7fda6372ffed586670573c59e07a48c86d606bab05db118abe0ef5
Status: Downloaded newer image for debian:stretch
 ---> 2d337f242f07

...which appears to indicate the BASE_IMAGE that will be used is from ARG BASE_IMAGE=ubuntu:18.04, but reading more carefully I can see the right thing is happening.

This looks good to me, there's only one part I'm not sure about and you called it out below, I'll comment there.

kingdonb

I think this is good to merge

…test

matyasmarkovics · 2019-04-17T08:17:16Z

Sorry for the update after your approval, I just wanted to have a fix for that ttyname failed warning. I've based the fix on: https://superuser.com/questions/1160025/how-to-solve-ttyname-failed-inappropriate-ioctl-for-device-in-vagrant

The issue was that apparently the default remote command of ssh produces this warning. The default remote command was executed due to the test.sh script being sent-over as oppose to being copied. Now, a simple bash is invoked in the runner.sh to override the default remote command and to avoid that warning.

kingdonb · 2019-04-19T19:10:00Z

Thank you for the attention to my comments @matyasmarkovics, I think this looks good. Still needs to be rebased now before it can merge. Maybe if you do that, then we can call someone who can override the CI failure, since it's unfortunately not in my power to merge stuff when CI is failing...

matyasmarkovics · 2019-04-19T20:01:38Z

I've just checked no rebase is needed as the master branches are up-to-date. The CI fails at the time of waiting for credentials to docker-hub.

kingdonb · 2019-04-27T13:13:17Z

It looks like I have had my privilege escalated, I can merge it now! Is there anything else you think this needs before it goes into master branch?

matyasmarkovics · 2019-04-29T11:05:23Z

Hey @kingdonb thanks for taking the time reviewing these changes. It's good to go from my end.

kingdonb · 2019-05-05T20:05:33Z

I am mistaken, I don't have the ability to override the failing build and merge this on my own (failed build which appears to be simply due to outdated docker hub credentials.)

@Theaxiom Do you have time to review this and approve/merge?

yebyen · 2019-05-09T15:39:07Z

I'm going to work on this for a while at yebyen/baseimage-docker and try building it against debian:sid, and using it. If you're interested to submit more PRs against it before we merge this in, I'd suggest posting them against yebyen/baseimage-docker:master and I'll be reviewing them over there.

This might be something we want to spend some more time reviewing before it merges anyway.

Theaxiom · 2019-05-10T23:55:09Z

@kingdonb would you mind ensuring docs are up to date with these changes?

kingdonb · 2019-05-11T02:43:01Z

I will review everything 👍

Theaxiom · 2019-05-13T17:20:51Z

Thank you @kingdonb

matyasmarkovics added 7 commits March 12, 2019 23:28

build-arg to define base-image, fix locale for Debian

4f65f0a

if both NAME and BASE_IMAGE are defined, don't append variant to base…

089258a

…image

exclude mounts (e.g.: /proc) from cleanup

9d29b23

remove interactive and TTY flags when Enabling SSH in test

17a737f

ssh by IP in test; verbose SSH into container

ad88ec9

send tests over in ssh, instead of mounting them

4a3476b

invoke make from travis

6d6395c

matyasmarkovics commented Mar 31, 2019

View reviewed changes

matyasmarkovics added 2 commits April 8, 2019 09:45

Default to ubuntu BASE_IMAGE in Dockerfile, modify make to only add -…

36bd411

…-build-arg to build target if BASE_IMAGE is defined, document the use of BASE_IMAGE in README

Use tools/docker-ssh in make ssh and in test/runner.sh to connect to …

89597c5

…container via SSH, fix SSH connection by IP issue on Mac-OS

kingdonb reviewed Apr 14, 2019

View reviewed changes

kingdonb approved these changes Apr 17, 2019

View reviewed changes

fix for 'ttyname failed: Inappropriate ioctl for device' during make …

d616b3b

…test

kingdonb self-requested a review April 19, 2019 19:10

kingdonb approved these changes Apr 19, 2019

View reviewed changes

Theaxiom merged commit fc6672b into phusion:master May 10, 2019

Theaxiom added this to the 0.11.1 milestone May 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to override ubuntu:18.04 #527

Ability to override ubuntu:18.04 #527

matyasmarkovics commented Mar 13, 2019

kingdonb commented Mar 13, 2019 •

edited

Loading

kingdonb commented Mar 31, 2019 •

edited

Loading

matyasmarkovics commented Mar 31, 2019

matyasmarkovics Mar 31, 2019

matyasmarkovics Mar 31, 2019

matyasmarkovics Mar 31, 2019

matyasmarkovics Mar 31, 2019 •

edited

Loading

kingdonb Apr 14, 2019

matyasmarkovics commented Mar 31, 2019

kingdonb commented Apr 13, 2019

kingdonb left a comment

kingdonb left a comment

matyasmarkovics commented Apr 17, 2019 •

edited

Loading

kingdonb commented Apr 19, 2019

matyasmarkovics commented Apr 19, 2019

kingdonb commented Apr 27, 2019

matyasmarkovics commented Apr 29, 2019

kingdonb commented May 5, 2019

yebyen commented May 9, 2019

Theaxiom commented May 10, 2019

kingdonb commented May 11, 2019

Theaxiom commented May 13, 2019

		if test "$$ID" = ""; then echo "Container is not running."; exit 1; fi && \
		IP=$$(docker inspect $$ID \| grep IPAddr \| sed 's/.: "//; s/".//') && \

		sleep 1 # Give container some more time to start up.
		ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=no -i /tmp/insecure_key -p $SSHPORT [email protected] \

Ability to override ubuntu:18.04 #527

Ability to override ubuntu:18.04 #527

Conversation

matyasmarkovics commented Mar 13, 2019

kingdonb commented Mar 13, 2019 • edited Loading

kingdonb commented Mar 31, 2019 • edited Loading

matyasmarkovics commented Mar 31, 2019

matyasmarkovics Mar 31, 2019

Choose a reason for hiding this comment

matyasmarkovics Mar 31, 2019

Choose a reason for hiding this comment

matyasmarkovics Mar 31, 2019

Choose a reason for hiding this comment

matyasmarkovics Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

kingdonb Apr 14, 2019

Choose a reason for hiding this comment

matyasmarkovics commented Mar 31, 2019

kingdonb commented Apr 13, 2019

kingdonb left a comment

Choose a reason for hiding this comment

kingdonb left a comment

Choose a reason for hiding this comment

matyasmarkovics commented Apr 17, 2019 • edited Loading

kingdonb commented Apr 19, 2019

matyasmarkovics commented Apr 19, 2019

kingdonb commented Apr 27, 2019

matyasmarkovics commented Apr 29, 2019

kingdonb commented May 5, 2019

yebyen commented May 9, 2019

Theaxiom commented May 10, 2019

kingdonb commented May 11, 2019

Theaxiom commented May 13, 2019

kingdonb commented Mar 13, 2019 •

edited

Loading

kingdonb commented Mar 31, 2019 •

edited

Loading

matyasmarkovics Mar 31, 2019 •

edited

Loading

matyasmarkovics commented Apr 17, 2019 •

edited

Loading