Add a specific config struct for the update command (#18957)#19104
Add a specific config struct for the update command (#18957)#19104thaJeztah merged 1 commit intomoby:masterfrom
Conversation
|
I think update as a command is kind of a goofy name for the command, but I wasn't here a week ago to say that. I'd match the config to the name of the command. If the command is |
|
Design looks good, I'm +1 for this 👍 |
|
SGTM |
There was a problem hiding this comment.
Is there further work to do here to get rid of hostConfig? If we end up in a hostConfig anyway, are we really winning anything here?
There was a problem hiding this comment.
hostConfig here is invisible to users, I think that's the point.
There was a problem hiding this comment.
Yeah I didn't want to pass UpdateConfig down too much — I think daemon doesn't really need to know it.
|
Looks correct. |
|
it looks like #19116 is proving that keeping hostConfig is better than removing it completely. Tbh, I was going to agree with this change, but allowing to change the restartPolicy sounds like a good idea to me. So I'm leaning more towards not doing this. |
|
@calavera Adding support to RestartPolicy would be done to just add a new field to What I don't like now is that we pass the whole HostConfig and ignoring a whole lot of attributes (for good reasons)… but seeing the API, a consumer of this API could think "oh cool, I can update everything in When looking at this endpoint, it should be clear what is mutable in the container. By passing |
|
Collective review We're okay with this design to resolve #18957. Thanks @vdemeester |
|
@vdemeester rebase please :) |
c44bb56 to
5ee7a45
Compare
|
Rebased and added a probably temporary vendoring commit (cc @calavera) to make it build 😝. |
5ee7a45 to
f0389df
Compare
|
Rebased with the vendoring of |
|
So weird to only see a small piece of the puzzle :-) |
f0389df to
df7a625
Compare
|
LGTM |
|
@vdemeester not sure if this needs a documentation update too, or if that should be done somewhere else? |
|
Hum maybe remote api I think, I'll check once home ;-) |
df7a625 to
24209d7
Compare
|
Updated some API docs. |
|
Docs LGTM |
This allows to define clearly what is mutable or not in a container and remove the use of the internal HostConfig struct to be used. Signed-off-by: Vincent Demeester <vincent@sbr.pm>
24209d7 to
a4f6920
Compare
|
docs LGTM, thanks @vdemeester! |
Add a specific config struct for the update command (#18957)
This is a proposal for #18957, to reduce what we send to the
updateendpoint. This also make it clear what is mutable in the container, and untie it fromConfig/HostConfig. It is still easy to add more attributesThis allows use to control more what is or is not mutable in a container and remove the use of the internal HostConfig struct to be used 🐳.
UpdateConfigmight not be a good name, maybeMutableConfigor something ?Related PR for api changes : docker-archive-public/docker.engine-api#11. Waiting for it to be merged to vendor it and rebase this one 😝.
Closes #18957.
Signed-off-by: Vincent Demeester vincent@sbr.pm