Skip to content
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

Remove Image during ensureImageExists so it does not impact config-hash #9350

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

kmdm
Copy link
Contributor

@kmdm kmdm commented Apr 4, 2022

What I did

I have a service, let's call it s1 which is running and has a config-hash thus:

$ docker inspect s1 | grep config-hash
                "com.docker.compose.config-hash": "64b9300b107597a13422a90ce6139837163ebc9f456ed069c36476e25e828f50",

However, when I run docker compose config --hash s1 I get this:

$ docker compose config --hash s1
s1 fa7906e8981ec4cb1b6a31767a0ab4b016fea826433c81e2400d5ad4d84acb54

This appears to be because if service does not specify an Image (e.g. it has Build instead) this is not taken into account when running config --hash but is part of the hash that is set as part of the label.

Once I apply this change to the config command, the results are thus:

$ go run cmd/main.go -f /path/to/docker-compose.yml config --hash s1
s1 64b9300b107597a13422a90ce6139837163ebc9f456ed069c36476e25e828f50

This now matches the config-hash label.

Use case: It would be nice if scripts/tools could easily compare the running hash with the expected/config hash to determine whether an update is required or not.

Related issue
n/a

(not mandatory) A picture of a cute animal, if possible in relation with what you did
🐶 🤷‍♂️

@ndeloof
Copy link
Contributor

ndeloof commented Apr 7, 2022

Image name is not relevant as tags aren't immutable. Better use image digest. (see com.docker.compose.image label)
If this is approved, need to check impact on mustRecreate which already checks image has been updated
also related: #9357

@ndeloof
Copy link
Contributor

ndeloof commented Apr 7, 2022

Not sure about the version you've been using, but config --hash should already return the same hash used for container's label.
see #8960 and #9214

@kmdm
Copy link
Contributor Author

kmdm commented Apr 7, 2022

Hi,

See test case:

$ git log | head -3
commit db698562a96834438c99cdaa6c03a385ae2fd386
Author: Nicolas De Loof <[email protected]>
Date:   Thu Apr 7 12:13:30 2022 +0200

$ cat Dockerfile.test
FROM golang:1.17

$ cat docker-compose.yml
version: '3'
services:
  test:
    build:
      dockerfile: Dockerfile.test

$ go run cmd/main.go -f docker-compose.yml up -d
[+] Running 1/1
 ⠿ Container compose-test-1  Started                                                                                                                                                    2.3s

$ docker inspect compose-test-1 | grep config-hash
                "com.docker.compose.config-hash": "3a92c9538ecea49c6941a828be3888070a17240096116febd57d401ac118a9cd",

$ go run cmd/main.go config --hash test
test 635648d959d03dbc9aeb97d054429b71b34bd3a45aa62d348ca1bcdff736a24c

$ curl -L https://github.com/docker/compose/pull/9350.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   141  100   141    0     0   1342      0 --:--:-- --:--:-- --:--:--  1342
100   752    0   752    0     0   1652      0 --:--:-- --:--:-- --:--:--  734k
patching file cmd/compose/convert.go

$ go run cmd/main.go config --hash test
test 3a92c9538ecea49c6941a828be3888070a17240096116febd57d401ac118a9cd

On the current v2, the hash is only correct once the patch is applied.

I'm hoping the patch doesn't affect any other operation since it's in cmd/compose/convert.go and not pkg/ or internal/.

Alternatively, I suppose, you could stop the hash using ServiceConfig.Image when no image is specified in the docker-compose.yml, which I think is the cause of this.

@ndeloof
Copy link
Contributor

ndeloof commented Apr 7, 2022

ok got it.
This is caused by 58bfbbb, as we force image name even if service has none declared.
From this point of view your fix looks correct but it's not pleasant we have to re-implement this within cmd package.
IMHO would be better to remove line https://github.com/docker/compose/blob/v2/pkg/compose/build.go#L143 which isn't required AFAICT

@kmdm
Copy link
Contributor Author

kmdm commented Apr 7, 2022

@ndeloof Yes, indeed. I wasn't sure about that line so I was hoping to raise the test-case, explained with an example fix (which seemed less intrusive) and then see what someone more experienced with the codebase made of it.

Do you need the PR changed to remove that line instead or would you prefer to handle it yourself since the fix becomes a "simple" one line removal? (I have no knowledge on whether anything depends on that being set or not!)

@ndeloof
Copy link
Contributor

ndeloof commented Apr 7, 2022

As you reported and investigated this issue, please update your PR to remove this line so you get the credits for fixing this issue :D

@kmdm
Copy link
Contributor Author

kmdm commented Apr 7, 2022

Done, but I have only tested my specific use-case, I can do a bit more if required but not right now!

@kmdm kmdm changed the title Add image name during 'config --hash' Remove Image during ensureImageExists so it does not impact config-hash Apr 7, 2022
@milas milas merged commit 1d678b7 into docker:v2 Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants