*: add bottlecap, an optional entry point #182

ajeddeloh · 2018-10-22T20:48:34Z

Add bottlecap as the default entry point. It will handle launching the
coreos-assembler container and optionally binding in and rebuilding
certain aspects to aid development. This is in additon to the existing
flows.

Putting this up as WIP to get some feedback, will add docs before final version

cgwalters · 2018-10-22T21:07:42Z

Can you elaborate slightly on the usage of this? One would run it as root like ./bottlecap from a git clone of this repo?

bottlecap

cgwalters · 2018-10-22T21:05:06Z

bottlecap

+
+if [ "$dev" == 1 ]; then
+	volumes+="-v $script_dir:/host/src "
+	volumes+="-v $(go env GOCACHE):/root/.cache/go-build "


On a SELinux enabled system this assumes these are labeled too. Which may be OK, but will almost certainly bite people. (I try not to run containers accessing my /home for example)

hmm. the gocache is easy enough (just put it literally anywhere that will persist across container runs).

If we want to support local dev though we need to be able to bind in wherever the build scripts are coming from (which is right now, this repo)

The TL;DR is that SELinux + docker/podman (as root anyways) want files to be labeled specially for containers. Trying to do -v /home/myuser:/home for example will fail, and you don't want to relabel your /home.

(To pivot this topic though, while I haven't played with it the "rootless" podman should sidestep this issue; if we merge the bits to have this container not require privileges, just /dev/kvm, that will work well for rootless too)

another note here: we need to check if go env GOCACHE reports a value before we try to mount it in:

$ ./bottlecap --build-dir /srv/ --dev --container ca build ./bottlecap: line 81: go: command not found + sudo podman run --rm --net=host -ti --privileged --userns=host -v /var/srv:/srv -v /var/sharedfolder/code/github.com/coreos/coreos-assembler:/host/src -v :/root/.cache/go-build --workdir /srv --entrypoint /host/src/bottlecap-shim ca d invalid host path, must be an absolute path ""

ajeddeloh · 2018-10-22T21:12:32Z

Yeah, that's the idea. Basically allow an easy way to both work on the container itself and give an opinionated way to use it. Manually setting bash aliases is not ideal. The goal is also to keep bottlecap as stripped down as possible.

Edit: shouldn't need root I don't think?

bottlecap

dustymabe · 2018-10-23T13:40:28Z

bottlecap

@@ -0,0 +1,76 @@
+#!/bin/sh
+
+# bottlecap, 'cause it's kinda like cork... get it?


cork is nice since it's 4 letters :) - I like cass because it's also 4 letters, but I also like highlighting that this is the script vs an alias that someone has defined on the command line (since I like to define my alias as alias cass=. So cass.sh would be my suggestion here, but also takes us back to 7 characters so I'm a hypocrite :)

This was mostly just a thought excercise for me. We don't need to change anything unless you want to.

ajeddeloh · 2018-10-25T22:59:45Z

Not sure what to do about the selinux issue @cgwalters brought up (ideas?) but I switched to getopt parsing.

dustymabe · 2018-11-08T20:25:51Z

one thing you could do for the selinux issue is detect and error (or warn). If system has selinux enabled and the label on the files aren't going to work then error/warn. For example tell them they could chcon -Rt svirt_sandbox_file_t /path/to/files/

dustymabe · 2018-11-08T20:32:42Z

bottlecap

+	echo "  --container: which coreos-assembler container to use"
+	echo "  --help: print this help message"
+	echo "Command to be executed:"
+	runtime="echo $runtime"


I feel like the above two lines should be deleted.

I could go either way. It's certainly been useful for debugging bottlecap and I think it does a good job of illustrating that bottlecap is just a thin wrapper. What about a ---dry-run option instead?

right now it doesn't actually output the command to be executed I don't think. It outputs Command to be executed: and then sets the runtime variable equal to "echo $runtime", which doesn't print anything to the screen.

it sets runtime to echo $runtime because some of the args are not set when usage is called. Note it does not exit immediately after usage.

I see. Thanks for explaining. I did not understand what you were doing. We discussed that you could also achieve this with --dry-run

bottlecap

dustymabe · 2018-11-08T20:34:58Z

bottlecap

+
+usage() {
+	echo "usage:"
+	echo "bottlecap [--dev] [--runtime $runtime] [--build-dir $build_dir]  [--container $container] assemblerargs..."


did you mean to evaluate the above variables when we print stuff to the screen? If not then we should use single quotes and not double quotes here.

I do. You can do bottlecap --build-dir foobar --help and see your arg reflected in the help text, which I am a fan of personally.

I see, I haven't seen that very much in practice, but seems OK

dustymabe · 2018-11-08T20:35:40Z

bottlecap

+build_dir="$(pwd)"
+container="quay.io/cgwalters/coreos-assembler"
+
+TEMP=$(getopt -o 'dr:b:c:h' --long 'dev,runtime:,build-dir:,container:,help' -- $@)


you need to check the exit code of getopt and call usage() and exit if it failed.

dustymabe · 2018-11-08T21:03:34Z

bottlecap

+	entrypoint="--entrypoint /host/src/bottlecap-shim"
+fi
+
+$runtime run --rm --net=host -ti --privileged --userns=host $volumes --workdir /srv $entrypoint $container $@


maybe we should set -x right above this command

also, do we expect users to run sudo ./bottlecap ?

also, do we expect users to run sudo ./bottlecap

If needed. I'd rather leave that to the user to decide than than bake it into the script.

maybe we should set -x right above this command

SGTM

dustymabe · 2018-11-08T21:07:58Z

a few style comments:

there is a PR to run most of our bash through shellcheck, might be worth doing that here too.
you like tabs don't you? I think we had that conversation before

bottlecap

ajeddeloh · 2018-11-08T21:41:59Z

Will do on running it through shellcheck.

And yeah, I do like tabs, let everyone decide their own indentation level (I typically like 8, which is far more than most people I think).

ajeddeloh · 2018-11-09T00:20:59Z

Fixed a lot of things, switch the gocache to just always use $script_dir/.gocache and added that to the .gitignore.

Passes shellcheck, with one exception where we actually do want glob expansion

dustymabe · 2018-11-12T19:40:10Z

bottlecap

+rc=0
+TEMP=$(getopt -o 'dr:b:c:h' --long 'dev,dry-run,runtime:,build-dir:,container:,help' -- "$@") || rc=$?
+if [ "$rc" -ne 0 ]; then
+	echo "Bad arguments. getopt failed."


should the above go to stderr ?

bottlecap

dustymabe · 2018-11-12T19:45:14Z

bottlecap

+			break
+			;;
+		*)
+			echo "Error parsing args"


not sure if this section is needed any longer since we are using getopt, but if we keep this let's spit that message to stderr

dustymabe · 2018-11-12T19:52:10Z

a few final comments in the code on stderr.. otherwise LGTM.

will probably want to update this in a future PR to handle the 'unpriv' case, but we don't need to do that now

Add bottlecap as the default entry point. It will handle launching the coreos-assembler container and optionally binding in and rebuilding certain aspects to aid development. This is in additon to the existing flows.

ajeddeloh · 2018-11-12T20:43:34Z

Made errors go to stdout, fixed shellcheck complaints in bottlecap-shim

ajeddeloh force-pushed the bottlecap branch from 660056f to 50e4641 Compare October 22, 2018 21:06

cgwalters reviewed Oct 22, 2018

View reviewed changes

ajeddeloh force-pushed the bottlecap branch 2 times, most recently from 956db3b to 00af17e Compare October 22, 2018 21:57

dustymabe reviewed Oct 23, 2018

View reviewed changes

bottlecap Outdated Show resolved Hide resolved

dustymabe reviewed Oct 23, 2018

View reviewed changes

bottlecap Show resolved Hide resolved

dustymabe reviewed Oct 23, 2018

View reviewed changes

cgwalters assigned dustymabe Oct 23, 2018

ajeddeloh force-pushed the bottlecap branch from 00af17e to 30aa151 Compare October 25, 2018 22:52

dustymabe reviewed Nov 8, 2018

View reviewed changes

bottlecap Show resolved Hide resolved

dustymabe reviewed Nov 8, 2018

View reviewed changes

bottlecap Show resolved Hide resolved

ajeddeloh force-pushed the bottlecap branch 2 times, most recently from 0212211 to 4c30422 Compare November 9, 2018 00:18

ajeddeloh changed the title ~~WIP: *: add bottlecap, an optional entry point~~ *: add bottlecap, an optional entry point Nov 9, 2018

ajeddeloh force-pushed the bottlecap branch from 4c30422 to b4ae3a5 Compare November 9, 2018 20:08

dustymabe reviewed Nov 12, 2018

View reviewed changes

bottlecap Show resolved Hide resolved

dustymabe reviewed Nov 12, 2018

View reviewed changes

*: add bottlecap, an optional entry point

1548932

Add bottlecap as the default entry point. It will handle launching the coreos-assembler container and optionally binding in and rebuilding certain aspects to aid development. This is in additon to the existing flows.

ajeddeloh force-pushed the bottlecap branch from b4ae3a5 to 1548932 Compare November 12, 2018 20:42

dustymabe approved these changes Nov 12, 2018

View reviewed changes

dustymabe merged commit b6f3bb3 into coreos:master Nov 12, 2018

ajeddeloh mentioned this pull request Nov 13, 2018

consider seperating host bits from build root bits #75

Closed

cgwalters mentioned this pull request Nov 26, 2018

README and build: add info on testing modified script locally, and boldness to build output error #237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add bottlecap, an optional entry point #182

*: add bottlecap, an optional entry point #182

ajeddeloh commented Oct 22, 2018

cgwalters commented Oct 22, 2018

cgwalters Oct 22, 2018

ajeddeloh Oct 22, 2018

cgwalters Nov 1, 2018

cgwalters Nov 1, 2018

dustymabe Nov 8, 2018

ajeddeloh commented Oct 22, 2018 •

edited

Loading

dustymabe Oct 23, 2018

ajeddeloh commented Oct 25, 2018

dustymabe commented Nov 8, 2018

dustymabe Nov 8, 2018

ajeddeloh Nov 8, 2018

dustymabe Nov 8, 2018 •

edited

Loading

ajeddeloh Nov 8, 2018

dustymabe Nov 8, 2018

dustymabe Nov 8, 2018

ajeddeloh Nov 8, 2018

dustymabe Nov 8, 2018

dustymabe Nov 8, 2018

dustymabe Nov 8, 2018

ajeddeloh Nov 8, 2018

ajeddeloh Nov 8, 2018

dustymabe commented Nov 8, 2018

ajeddeloh commented Nov 8, 2018

ajeddeloh commented Nov 9, 2018

dustymabe Nov 12, 2018

dustymabe Nov 12, 2018

dustymabe commented Nov 12, 2018

ajeddeloh commented Nov 12, 2018

		@@ -0,0 +1,76 @@
		#!/bin/sh

		# bottlecap, 'cause it's kinda like cork... get it?

*: add bottlecap, an optional entry point #182

*: add bottlecap, an optional entry point #182

Conversation

ajeddeloh commented Oct 22, 2018

cgwalters commented Oct 22, 2018

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

ajeddeloh commented Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

ajeddeloh commented Oct 25, 2018

dustymabe commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe Nov 8, 2018 • 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

dustymabe commented Nov 8, 2018

ajeddeloh commented Nov 8, 2018

ajeddeloh commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Nov 12, 2018

ajeddeloh commented Nov 12, 2018

ajeddeloh commented Oct 22, 2018 •

edited

Loading

dustymabe Nov 8, 2018 •

edited

Loading