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
build: bump docker-compose to v2.24.5, set healthcheck.start_interval
to 1s
in Docker v25+
#5745
build: bump docker-compose to v2.24.5, set healthcheck.start_interval
to 1s
in Docker v25+
#5745
Conversation
Download the artifacts for this pull request:
See Testing a PR |
The new
Related issue: |
Pretty weird! Nice job chasing it. But it needs to be fixed somewhere or we're stuck with old compose. Seems like compose bug. |
Yes, I will see what can be changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, I'm probably going to be OK with it, but it does look fragile for the future. I'd sure rather it be fixed in docker-compose. Interested in doing a PR there?
pkg/ddevapp/config.go
Outdated
@@ -747,6 +747,7 @@ type composeYAMLVars struct { | |||
Hostnames []string | |||
Timezone string | |||
ComposerVersion string | |||
DockerVersion string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use DockerEngineVersion here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to use a different technique: introduce a new custom function templateCanUse(feature string)
for use in templates, which can perform more complex checks on the go side.
I think they made a reasonable change with error, because
Agree, and for this reason I added a check specifically for the Docker API version https://docs.docker.com/engine/api/#api-version-matrix |
Looking deeper into the code base, there is a check for the minimum version of Docker to use with DDEV: ddev/pkg/dockerutil/dockerutils.go Lines 1733 to 1739 in 2defedb
And it does some exotic things in comparison, like removing ddev/pkg/dockerutil/dockerutils.go Lines 772 to 783 in 2defedb
From what I can see, people who build Docker can override the Docker version as they wish, for example from the Arch package (people can set any I would rather set the minimum API version to use with DDEV - this value is set and controlled only by Docker. @rfay, what do you think about this? (Of course, this is beyond the scope of this PR, and can be done in a new PR). |
API version sounds like a fine thing to me. In general, we've been trying to support as much as we can all the time, and been pretty successful with it over the long term. |
templateCanUse
function for yaml
I've updated the PR description with more information about the changes made, it should be fine to merge. |
I'll try to manually test. Looks good to me also. |
I guess my major worry would be that since healthcheck.start_interval was previously ignored, it not being ignored when using Docker v25 might cause some new issues. |
This check |
templateCanUse
function for yamlhealthcheck.start_interval
to 1s
in Docker v25+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great to me. I tested against Docker 25.0.2 (Docker Desktop) and against Docker 24.0.7 (colima) and the behavior was correct.
The reason I was a little hesitant was that I have misunderstood these configuration values and misused them many times during DDEV's history :)
|
||
return m | ||
} | ||
|
||
// templateCanUse will return true if the given feature is available. | ||
// This is used in YAML templates to determine whether to use a feature or not. | ||
func templateCanUse(feature string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty brilliant, thanks.
The Issue
It's time to update
docker-compose
.The most significant change in v2.24.0:
Breaking change⚠️
service hash computation logic has been updated to fully ignore replicas/scale. Due to this change, after upgrade all services will be recreated.
How This PR Solves The Issue
templateCanUse(feature string)
for use in templates, which can perform more complex checks on the go side.healthcheck.start_interval
(default is5s
and we use1s
) when it is really available (only for Docker Engine API 1.44+, i.e. for Docker v25+) https://docs.docker.com/engine/reference/builder/#healthcheck