Build images in the presence of a Dockerfile, when "from" is omitted

[TomFrost]

To date, one of the defining points of Binci vs Compose was that Binci never built images. Getting its legs, this was a huge advantage as it significantly improved iteration time. In practice, this sucks for everything but Node (and sometimes also for Node) because project-specific (and usually native) dependencies either need to be re-installed with every run which takes an incredible amount of time, or managed in a separate Dockerfile built specifically for that project, which is an incredible pain in the ass.

Case and point: Binci requires this repo, and after this update, that Dockerfile can just be moved into the root of this project. 💥

With this update, Binci will:

• Allow from to be omitted from the config
• Run a docker build if a Dockerfile is present in the cwd, or if dockerfileis specified in the config
• Name the built image with the name of the parent directory, and tag it with bc_12charHashOfDockerfile
• Delete the previous most recently built image that matches that naming scheme
• Run tasks per usual. On the next run, Binci will see the image that it built and will not re-build unless the Dockerfile is changed.

Design notes:

• We could have binci delete ALL previously built images that match its naming scheme after a successful build, but I chose to go with the most recent one because, if there are more than one, it's because Binci already failed to delete the previous ones and trying again would probably just make it continue to error out
• I separated the operations of loading the user config/merging with CLI params, and transforming that config into the runtime config that Binci needs. This allows us to plug in "middleware" to modify the project config before it's translated into CLI commands. This is where I injected the piece to create a "from" field with the name:tag of the newly built image.

Still TODO:

• Modify the init command to not force a from selection if a Dockerfile is present in the cwd
• Add a dockerfile override arg to the CLI parser
• Fix the system tests? (@Fluidbyte binci e2e is failing for me in the master -- did this work at all?)
• Make the readme clearer about this change? It currently takes the tack that image building is a fringe feature that happens if you don't take the default path of specifying a from. But this may be a big enough deal to make specifying a from the exception.

[codecov-io]

Codecov Report

Merging #88 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #88   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     11    +1     
  Lines         305    373   +68     
=====================================
+ Hits          305    373   +68

Impacted Files	Coverage Δ
src/args.js	100% <ø> (ø)	⬆️
src/config.js	100% <100%> (ø)	⬆️
src/index.js	100% <100%> (ø)	⬆️
src/init.js	100% <100%> (ø)	⬆️
src/command.js	100% <100%> (ø)	⬆️
src/images.js	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cc56ad...f2bc825. Read the comment docs.

[ksafranski]

A couple notes just from my immediate look at this:

• Explicit declaration 👍 ; never assumed based on a Dockerfile in root. I just feel like this could be problematic. Binci has always maintained a low learning curve because the config was designed to be obvious.
• E2E tests can be tricky. I haven't established why this is problematic, but my theory is it has to do with dependencies of the test project. cd into the test project directory and run an install first and then typically there aren't any issues.

[ksafranski]

perhaps instead we assume an image pull, if the desire is to build, from goes from from: some-image:latest to from: file://path/to/Dockerfile.

[TomFrost]

See top-level comment!

[ksafranski]

-f in here may prevent some issues...

[TomFrost]

I played with this when I was using the image ID to do deletes -- the issue with -f is that it forces a delete even when it's an intermediate of another container. It's not the smoothest move in the world for someone to base a new container on top of a binci build, but if someone did, throwing in -f would delete their work. That's why I went with the only-delete-the-most-recent model -- if one fails to delete (because someone referenced it elsewhere) then that will be the only time it fails.

[ksafranski]

Since you'll be working with this feature most directly I assume you'll catch if there is a benefit in the -f flag down the road (just assuming no one would be strangely building off binci-gen'd images). I just remember how stubborn Docker is on some environments (cough cough OSX) with things like stop and rm.

[TomFrost]

hahaha, thankfully I've not seem the same hangup on the images side compared to the container side. It just seems to bitch (Without the -f) if you're trying to delete an image with dependents, or if you're trying to delete an ID that has multiple tags.

[TomFrost]

Explicit declaration 👍 ; never assumed based on a Dockerfile in root. I just feel like this could be problematic. Binci has always maintained a low learning curve because the config was designed to be obvious.

The Dockerfile-in-root thing is a fallback -- it only checks for that if a from doesn't exist, as a path of last resort before failing. I see what you mean about explicit declaration though... I see two paths:

• Keep it as-is where from refers to a built container, and dockerfile refers to a path to a Dockerfile to use, defaulting to './Dockerfile'. But, allow dockerfile to be overridden on the CLI too, just like from, to make them equal options at the top level.
• Eliminate the dockerfile parameter and use from to refer to both image name:tags, and to files. You suggested file://... above, but I'm not a huge fan because not only is that an awkward format that doesn't make it immediately obvious that relative paths are supported, it also steeply implies that any valid URI is supported... and we don't have plans to support http or others. But we COULD just detect the string starting with . or path.sep and immediately infer that it's a file path based on that.

Either way, I think it's a good move to, if from is omitted, attempt to build rather than immediately fail. In addition to being generally friendlier/simpler for the user, Compose files also don't have a from and it would make the transition easier for those users.

I'm personally more a fan of the first option (:thumbsup: to avoiding string processing where possible) but I'd be open to the second if you think a second optional parameter is too much complexity.

[ksafranski]

I'm fine with it. Probably just being picky about things, but realistically it makes it easier to fallback if no from is supplied.

[TomFrost]

👍 Added TODO item

[TomFrost]

Still no love on the e2e tests after trying your suggestion... I keep running into:

#----------------------------------------------
# basic: Runs the 'env' task
# binci env
#----------------------------------------------


✖ Starting services mongodb, redis
✖ Failed to start services: mongodb, redis

And that's the result of pretty much every test :-/

[TomFrost]

All other WIP items completed

[ksafranski]

This is going to seem so nit-picky it's gross, but can you reverse this so from is the first topic, dockerfile the second. My reasoning is; 1. binci init creates with from, and 2. there hasn't been a voiced need for this until now (at least not one acted upon) so I think the majority use-case is from.

[TomFrost]

Hahaha, flipped in latest push. Though, to counterpoint (because I have to :D)

1. init also writes dockerfile now in addition to from.
2. I think the only reason the majority use case is from is because that's the habit we instilled. Testing your own Dockerfile/running tasks against a prod-like build is a generally Good Thing (how many times did we forget to update something in our Dockerfile in the past couple years and not figure it out until it got all the way to staging and crashed), and needing an extra step of maintaining an external base image for a project that requires anything custom at all is a step we got used to/comfortable with, but it's additional work that's now solved by just putting that Dockerfile in your project repo and not using from anymore. This repo right here is a great example. The repos at our former employer could have made sense under this model too, considering some of them had to run apk add before most tasks and others just included their deps in the image we maintained globally even if other projects didn't need them. I think it's friggin phenomenal that Binci doesn't force you to build if you don't want to, but I think building by default is one of the things Compose got right. Handling how and when that happens, not so much... but the build is good.

[ksafranski]

Looks good to me (minus the nit fix). If you're good with it, I am.

[ksafranski]

I've been out all day, but I can pull it and run E2E's to see what's up.

[TomFrost]

Snuck in one more feature... Since Dockerfiles generally copy the codebase in to the image at a specific path, I added the workDir config property to allow you to change the mount point (and obviously working directory) where the CWD gets mounted. That way, you can mount it over top of the path where the source files get COPYed by the Dockerfile if you want to run tests using the same paths you'd use in prod.

If we're throwing in the towel on making sure this didn't break e2e, I'm good with a merge after this!