Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

Updating ContainerConfig to use engine-api #1983

Merged
merged 15 commits into from
Apr 14, 2016

Conversation

nishanttotla
Copy link
Contributor

This is work in progress, PR is meant for review

This is the first change that will truly affect the entire repo AND the users, and will need discussion as some old API conventions might have to be deprecated.

The important decision here is the change in ContainerConfig struct inside config.go. Initially, ContainerConfig looked like

 type ContainerConfig struct {
     dockerclient.ContainerConfig
 }

but now it looks like

 type ContainerConfig struct {
     container.Config
     container.HostConfig
     network.NetworkingConfig
 }

where container and network are packages in engine-api/types. This is keeping in line with the recent Docker API changes where HostConfig and NetworkingConfig was moved out of ContainerConfig (renamed as Config) to be independent.

"This change is too big"

I'm afraid so, but there's no other way to do it, all of this change needs to happen at once. Three function calls have been moved over to use engine-api, namely ContainerCreate, ContainerInspect, and ContainerList, along with the relevant changes needed to make these work.

"How do I review this?"

Fortunately, many changes can be classified as "plumbing", so the root changes are fairly small. Start with cluster/config.go and cluster/container.go to see how structs have changed. Then check out cluster/engine.go where all the real stuff is happening, and scan through the calls being made to ensure that it all makes sense. Note: We don't have an embedded HostConfig inside Config anymore, they're separate, so we don't have to copy data over each time.

Finally, please do pull this branch and test locally, to make sure that it actually works.

This PR is part of a series to implement #1879.

Signed-off-by: Nishant Totla nishanttotla@gmail.com

@MHBauer
Copy link
Contributor

MHBauer commented Mar 17, 2016

This was the first thing I hit, and I just nope'd out of there into easier territory. I had something similar to your updated ContainerConfig, so I think we were on the same path.

@nishanttotla
Copy link
Contributor Author

@MHBauer That sounds like my experience 😺. I think this change is the single major one that will take the longest to get right, the rest will either depend on it, or are more isolated. The difficulty here is that it affects a large part of the code, so I'm facing merge conflicts all the time!

@nishanttotla nishanttotla force-pushed the container-config-engine-api branch 14 times, most recently from 6c2ca36 to 46bc92c Compare March 23, 2016 19:00
@nishanttotla nishanttotla added this to the 1.2.0 milestone Mar 23, 2016
@nishanttotla nishanttotla force-pushed the container-config-engine-api branch 6 times, most recently from c069eec to 62af821 Compare March 25, 2016 00:28
@nishanttotla
Copy link
Contributor Author

This PR is currently blocked by docker/engine-api#171

@dongluochen dongluochen removed this from the 1.2.0 milestone Apr 8, 2016
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
response

Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@dongluochen
Copy link
Contributor

LGTM

1 similar comment
@vieux
Copy link
Contributor

vieux commented Apr 14, 2016

LGTM

@vieux vieux merged commit a764f9a into docker-archive:master Apr 14, 2016
@vieux vieux deleted the container-config-engine-api branch April 14, 2016 18:48
@MHBauer
Copy link
Contributor

MHBauer commented Apr 20, 2016

Wow, I didn't realize this was completed. Nice job!

@nishanttotla
Copy link
Contributor Author

@MHBauer thanks! Not much more left now!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants