New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing in CLI args directly via Docker #1252

Merged
merged 14 commits into from Aug 29, 2018

Conversation

Projects
None yet
5 participants
@ryankemper
Copy link
Contributor

ryankemper commented Aug 21, 2018

I found that while brakeman works great when gem installed locally, the Dockerbuild file is set up such that you can't pass in arbitrary arguments to brakeman. Regardless of input, it simply supplies the default output as JSON.

This branch changes CMD->ENTRYPOINT so that the user doesn't have to supply the path to the brakeman binary (which they shouldn't have to know about). Furthermore, it links directly to the brakeman binary rather than codeclimate-brakeman, since codeclimate-brakeman does not seem to respect command line arguments.

Now you can simply build the docker image and run it just like you would run brakeman locally, e.g. a command like the following:

docker run -v "$(pwd)":/code brakeman -o brakeman_docker_w3.html -q -w3

Thanks

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 21, 2018

Hi Ryan, thanks for the input!

Since that Dockerfile is specifically for the Code Climate engine, probably can't just change that :)

I would like to see a Dockerfile that supports what you are trying to do, but I guess it would have to be in parallel with the Code Climate engine.

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 21, 2018

Hey presidentbeef,

Thanks for the quick feedback. Would it be acceptable to have an alternate Dockerfile, like (I'm bad with names) Dockerfile.CLI or something?

If so, I'd be happy to make that change an add a corresponding blurb to the README with instructions on how to build with the alternate Dockerfile

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 21, 2018

Doesn't matter much to me. Would likely make more sense to have the default Dockerfile run Brakeman and a separate one for Code Climate, but I'm not sure how that would work.

@noahd1 can you comment?

@noahd1

This comment has been minimized.

Copy link
Contributor

noahd1 commented Aug 22, 2018

I'm almost certain that either way will work for us, as long as at the end of the day we can build a Docker image, which is indeed possible regardless of the name of the Dockerfile. @dblandin can you confirm, and also add any insight into naming conventions, if there are any?

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 22, 2018

Thanks for following up. I can think of two possible options:

  • rename Dockerfile => Dockerfile.codeclimate and use the Dockerfile in this branch as the default, and provide the docker build command w/ Dockerfile.codeclimate as a blurb in the README.

  • rename the dockerfile of this branch => (DOCKERFILE.CLI or DOCKERFILE_CLI or similar), and provide the docker build command w/ the alternate dockerfile as a blurb in the README.

After thinking it over I think the first option is the cleanest, since the Dockerfile gives access to the same set of CLI args as the corresponding ruby gem, so I would strongly favor that option. However if there's a bunch of external dependencies that expect the Dockerfile to use the code-climate options then I'm perfectly willing to go with the latter option.

Once you guys have consensus just let me know and I'll implement the changes.

EDIT: I thought it over a bit more and updated my original proposals, so please take another look at this comment if you'd already read it

@dblandin

This comment has been minimized.

Copy link
Contributor

dblandin commented Aug 22, 2018

Renaming the Code Climate Dockerfile to Dockerfile.codeclimate sounds great to me 👍

Our release process looks for a Dockerfile by default but also supports a custom build command so we can easily add docker build --file Dockerfile.codeclimate . for our brakeman entries.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 22, 2018

This is awesome, thank you @noahd1 and @dblandin for jumping in.

Let's move Dockerfile to Dockerfile.codeclimate as suggested.

That also means the default Dockerfile can be independent of any of Code Climate's requirements. Are there any other changes that might make it easier to use? (I'm not a Docker expert).

@ryankemper ryankemper force-pushed the ryankemper:master branch 2 times, most recently from 1830dc8 to 4c5fa60 Aug 22, 2018

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 22, 2018

I've implemented the above. I also went ahead and updated the README, but I wasn't completely sure where to add the new blurbs so let me know if you guys think I should make any further changes.

Currently I'm working on making sure the commands I put in the README work - I think instead of -o brakeman_results.html I might have to put -f html ... > brakeman_results.html, so I'll investigate and report back in a bit.

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 22, 2018

Quick update: It does look like you need to redirect output of the docker container like so:

docker run -v "$(pwd)":/code brakeman -q -f html > brakeman_results.html

It makes intuitive sense to me why this is required (I imagine the output file is being written inside the container), but weirdly I thought I had it working with the -o flag yesterday. But I think my memory might be failing me there. Anyway, in the next commit I'll update the README to tell the user a) to include the -q flag so we can pipe stdout and b) use -f html > brakeman.html instead of -o brakeman.html.

So, that's one point of difference between the command syntax on the ruby gem vs with the dockerfile.

Let me know if you guys think of anything else to add, and also if anything I wrote above looks incorrect.

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 22, 2018

Update: I had a talented coworker take a look at this PR and they had a few suggestions for the Dockerfile, so I just pushed some commits out to rearrange the order of the layers to avoid having to reinstall dependencies for every code change. Currently those changes were only made to Dockerfile and not Dockefile.codeclimate, so after testing I'll circle back and add the changes there too.

With respect to the -o command, I figured out that it does work in Docker, but if you supply a directory instead of using $(pwd) then the file is written there. So, I won't be changing the README command, but I will need to add a note explaining that.

Taking a break for a couple hours, then I'll be back to work on getting this wrapped.

@ryankemper ryankemper force-pushed the ryankemper:master branch 2 times, most recently from 3eb7089 to 04c84dc Aug 22, 2018

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 22, 2018

Current status:

The new Dockerfile appears to be working great, caches are working as expected. Once you guys have a chance to take a look, we can discuss porting the same changes to Dockerfile.codeclimate (which might end up saving the code-climate CI module some $$$ although I'm not sure).

There's one last issue - Docker volumes persist on the users' machine after the container is destroyed. While this is expected behavior as far as persistence goes, as far as I can tell the container doesn't actually need persistence for what it's doing.

For example, running docker volume ls shows an entry like local efb4eca5df4ad0678b45aa24030f543906b0cce24f1ca2f2a5a6a57d18dd4009 for every time I've ran the docker image.

If I can find a clean way to fix the issue, I'll include it in this branch, otherwise I'll leave it as a future improvement.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 23, 2018

Will adding --rm handle the volume issue?

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 23, 2018

From reading the docs, it looks like --rm has to be ran by the user, as opposed to it automatically being ran when the container tears down.

I think we can get away with just not having the volume commands in the Dockerfile at all; my understanding is the -v flag works regardless of the docker volume command but I haven't had time to verify.

[Meta: I probably won't be able to return to this pull request until tomorrow or so]

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 24, 2018

Update: Manually running docker volume rm would work, but the volume (and associated containers) are actually still hanging around, so docker doesn't let me. I had to delete all docker containers like so:

docker_remove_containers() {
    docker stop $(docker ps -aq)
    docker rm $(docker ps -aq)
}

And then run docker rm on each volume. (There's probably a more elegant way to do it but I just piped docker volume ls to xargs after adding some commands to chop out the non-volume output).

Naturally, this is not ideal. The good news is, removing the docker volume statement entirely from the dockerfile fixes it, and still allows using -v.

I just pushed that change.

EDIT: I see, you meant add --rm to the run command, not doing a docker volume rm. Regardless, we don't need the volume so there's no reason to have it. But theoretically if we had to have a docker volume, adding --rm to the run command in the README would solve it.

At this point the Dockerfile is either ready to ship or very close to it, so please take a look when you guys get the chance. Thanks again!

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 27, 2018

This seems to be working great for me. Will merge soon unless anyone objects.

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 27, 2018

Awesome. I'll quickly make the last remaining changes:

  • remove the TODO I accidentally left in the Dockerfile
  • Apply the same Dockerfile changes to Dockerfile.codeclimate
  • Add readme blurb w/ how to build the code-climate image

Lastly, I'll rebase if necessary - haven't checked if anything's been merged to master since this PR has been opened

ryankemper added some commits Aug 21, 2018

Run brakeman as entrypoint, not CMD
This allows supplying the arguments to brakeman, e.g. `-o
sca_result.html`, without having to prepend with the path to
/usr/src/app/bin/codeclimate-brakeman.

In other words, this is what you have to do currently to run
brakeman on the current directory with arguments:

  docker run -v "$(pwd)":/code brakeman /usr/src/app/bin/codeclimate-brakeman -o output.html

and this is what you can do by using ENTRYPOINT instead of CMD:

  docker run -v "$(pwd)":/code brakeman -o output.html
Allow CLI args by directly running brakeman binary
(codeclimate-brakeman reads the config file, but ignores CLI args)
mv codeclimate dockerfile->Dockerfile.codeclimate
This restores the functionality of the codeclimate Dockerfile, but names
it Dockerfile.codeclimate so that the user can use the default
Dockerfile to run brakeman with access to the full set of command-line
arguments.

[Metadata: I could have rewritten history to `git mv` the original
Dockerfile -> Dockerfile.codeclimate and then added the new default
Dockerfile, but I felt it was easier to just re-add the original file
under this new name. Hopefully the commit history isn't too confusing.]
Update README.md wrt new default Dockerfile
Small meta-note: I put the docker build command in the "installation"
section, but it could also go in the "build" section.
Change deprecated MAINTAINER to use label
From https://docs.docker.com/engine/reference/builder/#maintainer-deprecated:

The MAINTAINER instruction sets the Author field of the generated images. The LABEL instruction is a much more flexible version of this
and you should use it instead, as it enables setting any metadata you require, and can be viewed easily, for example with docker inspect.

Meta-note: We might want to add the associated e-mail instead of just first+last name
Optimize Dockerfile for caching layers
* We want the code to get copied in as late as possible, to avoid
needing to re-download all the gems just to update the brakeman source
code.

* Also add some documentation to the lines.

Meta-notes:
  - Still need to figure out what `-D app` does before we're ready to merge.
  - Also going to investigate if the docker VOLUME command leaves
  volumes on the user's machine after `docker run`.
Copy gem-related files early in build
My earlier attempt to reorder the cache layers (see 2 commits ago) ended up breaking the
bundle install step. Oops.

Now we manually copy over the Gemfile, gemspecs, gem_common.rb and
lib/brakeman/version.rb. Collectively these represent everything bundle
needs.

Now the desired effect has been achieved - making a change to the source
code does not require re-running `bundle install`

ryankemper added some commits Aug 24, 2018

Remove VOLUME cmd
Docker volumes hang around after containers are deleted, which we don't
want since brakeman only needs the code to be passed in as a logical
directory which the `-v` flag seems to do.

Docker run command is the same as before since we left /code as the
workdir
Remove dangling TODO
Looks like the -D flag changes the defaults to what was set in the other
option flags? Still not 100% positive but the comments are good enough
for now.

@ryankemper ryankemper force-pushed the ryankemper:master branch from c589b6a to eb9d173 Aug 27, 2018

@ryankemper

This comment has been minimized.

Copy link
Contributor Author

ryankemper commented Aug 27, 2018

Added the changes, rebased to upstream, and force pushed. If the CI looks good, my recommendation is to :shipit:

@jadametz
Copy link

jadametz left a comment

👋 Hey everyone! Just wanted to give my $0.02 that this is an awesome change and say thank you for allowing us (Invoca, and more specifically Ryan) to contribute so easily.

This is going to be a great addition to our CI pipeline very soon 😄

@@ -1,15 +1,25 @@
FROM ruby:2.4-alpine
MAINTAINER Justin Collins
LABEL maintainer="Justin Collins"

This comment has been minimized.

@jadametz

jadametz Aug 27, 2018

@presidentbeef not sure if you wanna give this level of detail, but normally the Maintainer would come with some form of a contact method, e.g. email address.

This comment has been minimized.

@presidentbeef

presidentbeef Aug 29, 2018

Owner

Thanks, I will update that after merge.

@presidentbeef presidentbeef merged commit ac689aa into presidentbeef:master Aug 29, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Repository owner locked and limited conversation to collaborators Oct 16, 2018

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