Dont build image if it has already been pushed #885
Conversation
npezza93
force-pushed
the
check-if-already-built
branch
from
2448b17
to
48648c4
Compare
|
@djmb How does the registry work in tests? |
|
@npezza93 - the integration tests use docker compose to set up a few containers:
This creates a mini environment we can deploy to and access the hosts from. The docker registry is running in insecure mode. Any images that we are going to pull from docker hub (e.g. the nginx one) are copied across once and retagged to be pulled from the local registry. This is because otherwise you can hit docker hub rate limits pretty quickly if you are running the tests a lot. |
|
The test failure looks like some transient Docker issue. @npezza93 - this change makes sense, but the one issue with it is that the pre-build hook will be skipped if you don't have to build the image. While that's probably the right thing to do, there may be people who are doing things they want to happen on every deployment in that hook. As such I think we'll wait to add this to the 2.0 release (which should be happening fairly soon) and include it in the upgrade notes. |
npezza93
force-pushed
the
check-if-already-built
branch
4 times, most recently
from
fbc976c
to
1a08ea3
Compare
|
@djmb Sounds good! Thanks for the info. Decided to just stub the call to manifest to force it to always be false which looks to have resolved the test failures. |
npezza93
force-pushed
the
check-if-already-built
branch
2 times, most recently
from
dade60d
to
43cb183
Compare
test/integration/main_test.rb
Outdated
| @@ -10,7 +10,13 @@ class MainTest < IntegrationTest | |||
|
|
|||
| assert_app_is_down | |||
|
|
|||
| kamal :deploy | |||
| mock = Minitest::Mock.new | |||
There was a problem hiding this comment.
I don't think we should be mocking things in the integration tests. We should just test the results of the commands.
In any case kamal :deploy actually runs docker compose exec deployer kamal deploy, so I'm not sure the mocking is doing anything.
We could add a second redeploy of the second version later on and add new assert_hooks_did_not_run assertion after it to confirm the pre-build hook was not called, which would be enough to infer that there was no rebuild.
test/integration/main_test.rb
Outdated
| @@ -19,7 +25,7 @@ class MainTest < IntegrationTest | |||
|
|
|||
| kamal :redeploy | |||
| assert_app_is_up version: second_version | |||
| assert_hooks_ran "pre-connect", "pre-build", "pre-deploy", "post-deploy" | |||
| assert_hooks_ran "pre-connect", "pre-deploy", "post-deploy" | |||
There was a problem hiding this comment.
Does this not call the pre-build hook? I would expect it to because we are deploying a new version here.
npezza93
force-pushed
the
check-if-already-built
branch
2 times, most recently
from
eb248dc
to
64daac4
Compare
|
Good call @djmb. Should be fixed now! |
npezza93
force-pushed
the
check-if-already-built
branch
from
64daac4
to
469d925
Compare
|
An interesting use case here could be building the image outside kamal, but then using kamal to deploy that image. |
This is useful when you push a branch to different environments at separate times from CI. It's also useful if you have CI setup to deploy main but have a concurrency key setup. If you push quickly youll end up deploying the same commit twice and the second time the image doesnt need to be built since it already exists.