Updates #13
Updates #13
Conversation
pointlessone
commented
Oct 13, 2017
pointlessone
commented
- Bump up deps
- Stop packaging test deps, tests and some other support files into the final image
codeclimate-hermes
requested review from
toddmazierski
and removed request for
codeclimate-hermes
Makefile
Outdated
| --workdir /usr/src/app \ | ||
| --volume $(PWD)/spec:/usr/src/app/spec \ | ||
| $(IMAGE_NAME) \ | ||
| sh -c "bundle install --with=test && rspec" |
There was a problem hiding this comment.
This approach doesn't seem very desirable either, since it means reinstalling test deps on every test run.
We ship lots of engines with test deps installed, so personally I think the old way was fine: the test deps are usually a pretty negligible size increase.
I understand wanting to minimize the shipped engine size. If you want to keep test deps out of the shipped image, you could have a second codeclimate-grep:test image that gets built here and includes test deps, though it would have to be a separate docker image, not an image built on top of the existing one (otherwise you'd lose the cache everytime you changed a file & reinstall tests anyway, defeating the idea of keeping local test runs fairly fast).
There was a problem hiding this comment.
I think the issue can be partially avoided if tests are not copied into the image but mounted instead. It would still trigger image rebuild when engine source is changed but so will do the build-all approach. Test mounting is, actually, faster than rebuilding so with this approach you can iterate faster on tests.
Check out the last commit.
I don't think a whole separate test image is not a viable alternative. It would test a completely different image. You may miss some essential files from prod image but still can pass tests because test image is built completely from scratch. There's a decent chance Dockerfiles may drift apart over time since the prod one is not actually tested with this approach.
I think building on top of prod image is less error prone.
|
@wfleming This engine doesn't have patrick set up. Should I add it? |
There was a problem hiding this comment.
Clever solution, @pointlessone.
One small style nit. And I saw a fixup commit so please make sure you squash when merging.
This engine doesn't have patrick set up. Should I add it?
Sure! But I think that can be a separate PR.
|
|
||
| COPY . ./ | ||
|
|
||
| RUN chown -R app:app . && \ | ||
| ln engine.json / | ||
|
|
There was a problem hiding this comment.
💄 extra blank line here, we usually only use single blank lines between blocks in dockerfiles.
Don't package test deps, tests and a few other support files.
