-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
Thanks for this PR! Vendoring is correct; there may a bunch of spurious files which are typically removed by glide-vc, have you seen the helper script to help updating vendored libraries? Regarding the original |
a7b6603
to
a266cdd
Compare
a266cdd
to
475b77f
Compare
Ran the helper script, the extra files should be removed now. |
Yes, thanks. I'll have a look around to find out what's the deal with the non-nil exec check, and then come back to this PR for a review, sorry for delaying. |
After a bit of history checking, I think this was introduced at 973d851 and it has been growing out of control from there on. According to aci spec the "exec" field is optional so it is fine to proceed populating other fields if this one is empty. Thanks for bringing up attention around this inconsistency. |
@@ -62,6 +62,7 @@ type DockerImageConfig struct { | |||
NetworkDisabled bool | |||
MacAddress string | |||
OnBuild []string | |||
Labels map[string]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.
Note to self: this is coming out of https://github.com/docker/docker/blob/master/api/types/container/config.go#L58.
Double-checking from my side is finally over, and this LGTM. I just want to page @euank and @jonboulle here: userLabels are image properties and can be set at build/conversion time (like here). This means that in rkt/k8s we may end up promoting image labels to runtime k8s labels (selectors); I'm not sure if we want this behavior, but my stance is that it should be discussed in rkt/rktlet, as even without docker2aci users may still provide directly built appc/oci images with labels. |
LGTM in itself, and I agree I think we need to solve this in rktlet/CRI in any case. |
LGTM, I don't think that those image labels get raised into k8s at all currently (or in the near future), and I agree that this shouldn't be related so long as we all agree this makes sense just in the context of docker->aci semantics. Which I think we do. Merge at will (I would if I could) |
@julia-stripe thanks for the contribution! |
@euank invite sent so you can in future |
this is a second version of #209!
I started out by just converting Docker labels to UserLabels. But then I realized there was a problem! Sometimes, when the Docker image doesn't have a
Cmd
in it, the labels wouldn't get converted.So I also changed it to not require an image to have a Docker command to convert the other Docker metadata on the image. I can't see why this wouldn't work, but probably there's some reason I don't know yet that this
exec != nil
check was there in the first place.(also I might have gotten the vendoring wrong -- I haven't used glide before)