Handle case of configs on old daemon #162
Conversation
Codecov Report
@@ Coverage Diff @@
## master #162 +/- ##
==========================================
- Coverage 44.85% 44.83% -0.02%
==========================================
Files 171 171
Lines 11464 11468 +4
==========================================
Hits 5142 5142
- Misses 6027 6030 +3
- Partials 295 296 +1 |
cli/compose/convert/service.go
Outdated
| if err != nil { | ||
| return nil, errors.Wrapf(err, "service %s", service.Name) | ||
| var configs []*swarm.ConfigReference | ||
| if len(service.Configs) > 0 { |
There was a problem hiding this comment.
What if we tied docker-compose file-version to API version? (also dirty perhaps); i.e. when attempting to use docker-compose file format 1.3.x, produce an error if the supported API version is older then Y.
There was a problem hiding this comment.
Not sure if that's better, should think a bit about it 😓
There was a problem hiding this comment.
I'm not a huge fan of that though…
There was a problem hiding this comment.
I think that method is a lot more reliable.
I don't understand why this would be a special case. We added secrets before and didn't have this problem. Wouldn't we need to add this check to secrets as well (with a different version) ?
We should just error if API version is less than the minimum version required for the Compose file version.
There was a problem hiding this comment.
@dnephin secrets support were already there in swarm mode when we added them in compose v3. If we point to a 1.12 daemon (that may not have secret ?), the bug will be there for secret too yep.
There was a problem hiding this comment.
Oh, sorry, didn't see your comment.
I also was just looking how we handle other API endpoints that are not supported yet, and it looks like we have code to handle that, e.g.; https://github.com/moby/moby/blob/745795ef2e0089c5001e5a2fc7ba8c1ab0234857/client/container_prune.go#L16-L18
But looks like we didn't add such handling to the secrets and config API
cli/compose/convert/service.go
Outdated
| return nil, errors.Wrapf(err, "service %s", service.Name) | ||
| } | ||
| if versions.LessThan(version.APIVersion, "1.30") { | ||
| return nil, errors.Errorf("Server API version (%s) doesn't support configs declared in service %s", version.APIVersion, service.Name) |
There was a problem hiding this comment.
Can you make this error consistent with the other errors we have?
docker service ls requires API version 1.24, but the Docker daemon API version is 1.10
There was a problem hiding this comment.
I think we could fix the immediate issue without worry about the whole versioning thing for now.
In ParseConfigs() (and ParseSecrets()) we could exit early if len(requestedConfigs) < 0. That would skip any configs related API calls when there are no configs defined.
That fixes the immediate issue, and we can decide on how to handle API compatibility later?
If configs are declared for a service and pointing on an old daemon, error out properly (instead of "page not found"). If there is no configs declared, don't call convertServiceConfigObjs to avoid having an error. Signed-off-by: Vincent Demeester <vincent@sbr.pm>
vdemeester
force-pushed
the
150-configs-fixes-on-old-daemon
branch
from
5254250
to
cf5550c
Compare
|
Updated according to @dnephin comment 👼 |
If configs are declared for a service and pointing on an old
daemon, error out properly (instead of "page not found").
If there is no configs declared, don't call convertServiceConfigObjs
to avoid having an error.
Fix #150
Signed-off-by: Vincent Demeester vincent@sbr.pm