-
Notifications
You must be signed in to change notification settings - Fork 51
Add support for specifying multiple base images to use in the spec #492
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
Conversation
65d3fec to
87ac6a8
Compare
81e57da to
cabf8c6
Compare
| // which means we end up overwriting map keys when there are multiple windows | ||
| // platform images being output but with different OSVerions. | ||
| // platforms.FormatAll takes OSVersion into account. | ||
| func dcBuild(ctx context.Context, bc *dockerui.Client, fn dockerui.BuildFunc) (*resultBuilder, error) { |
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.
Is it really a good idea to fork this code? I generally think that's not a good idea, as we won't benefit from upstream changes if there are any.
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.
We have to fork it to get the functionality we need.
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.
Can these changes be submitted upstream? Then we wouldn't need to fork it.
I'm sure there's a way to preserve the old behavior while allowing the new.
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.
Here is how it could be done:
pmengelbert/buildkit@7552611
Whether or not that's acceptible upstream, I think we should at least commit to a plan to undo the fork if there's no practical way around forking.
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.
moby/buildkit#5614 upstreams this work.
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.
Can you add a link to moby/buildkit#5614 in the comment?
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.
Upstream is merged.
Looks like you have an issue already to unfork this. Seems like should be good?
MorrisLaw
left a comment
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.
First pass lgtm! Don't have a whole lot of context though. Left some nits and clarification questions.
This is pretty much to support building for multiple windows versions from windowscross but can also be useful (in the future) for allowing users to specify something other than a registry reference to provide an image. The actual support for multiple bases, where it can be supported, will come in a follow-up commit.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This is useful for when you want to have one image for different versions of Windows, given that for Windows containers the container OS must match the host OS. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Allows windowscross to produce a single multiplatform image using the specified base images.
Supersedes #480
Closes #480