Adding the ARG feature to build with any user for juju-1 #20

Merged
merged 4 commits into from Nov 28, 2016

Conversation

Projects
None yet
3 participants
Contributor

mbruzek commented Nov 17, 2016

  • Removed the run.sh script based on the work that was done in master.
  • Updated the README.md with the work done in master.

Just trying to keep this juju-1 branch fresh

Dockerfile
+
+VOLUME [ "$JUJU_HOME", "$JUJU_REPOSITORY" ]
+
+ADD setup.sh /setup.sh
@kwmonroe

kwmonroe Nov 17, 2016

Member

any reason you went with ADD over COPY here? docker docs say COPY is preferred if you're not relying on ADD auto extraction capabilities:

https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#/add-or-copy

@kwmonroe

kwmonroe Nov 21, 2016

Member

Since #21 went with COPY, please s/ADD/COPY here too.

-# The directory to look for charms.
-export JUJU_REPOSITORY=${HOME}
-# The path to Juju configuration files.
-export JUJU_HOME=${HOME}/.juju
@kwmonroe

kwmonroe Nov 17, 2016

Member

why remove these exports from the user's .bashrc? they seem handy to have set for anyone entering the jujubox.

@kwmonroe

kwmonroe Nov 17, 2016

Member

oh, i see in master that these aren't set in the .bashrc either.. do ENV doohickeys persist when the image is run? if so, i see why these .bashrc additions aren't needed.

@chuckbutler

chuckbutler Nov 28, 2016

Contributor

Yeah, ENV exports persist unless you run sudo, in which point anything in the env was going to be stripped anyway. 🧀

@mbruzek mbruzek changed the title from Adding the ARG feature to build with any user. to Adding the ARG feature to build with any user for juju-1 Nov 18, 2016

@kwmonroe kwmonroe referenced this pull request in juju-solutions/charmbox Nov 21, 2016

Merged

Reconcile with jujubox #66

Contributor

mbruzek commented Nov 22, 2016

Requested change committed, please re-review.

I had one minor comment. the rest of this looks good to me.

I validated the build routine as follows:

docker build --build-arg JUJU_USER=foobar -t jujubox-1 .

everything appears to be in order here. Just a minor nit about the juju-1 bin naming and you have no control over that. Maybe we could symlink it for the user so its transparent? is that a bad idea?

+
+# Set a welcome message in .bashrc with the exact version of Juju.
+RC=/home/$JUJU_USER/.bashrc
+echo "echo Welcome to jujubox version ${JUJU_VERSION}" >> $RC
@chuckbutler

chuckbutler Nov 28, 2016

Contributor

It might still be prudent to include information about how to run juju1

the binary has changed to juju-1, i was surprised when i fired up the assembled container, typed in juju and it responded with command not found :|

lameee

@mbruzek

mbruzek Nov 28, 2016

Contributor

Thanks for review. I thought about using a symlink, but I don't want to step on the packaging if they ever were to fix that.

@chuckbutler chuckbutler merged commit dc9961a into juju-solutions:juju-1 Nov 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment