-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support for outputing multiple windows images from one build #480
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Brian Goff <[email protected]>
7187467
to
0693f94
Compare
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.
Initial pass through. looking good (coming from my somewhat limited perspective)
frontend/build.go
Outdated
@@ -101,6 +125,17 @@ func BuildWithPlatform(ctx context.Context, client gwclient.Client, f PlatformBu | |||
return rb.Finalize() | |||
} | |||
|
|||
// BuildWithPlatform is a helper function to build a spec with a given platform | |||
// It takes care of looping through each tarrget platform and executing the build with the platform args substituted in the spec. |
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.
super nit:
// It takes care of looping through each tarrget platform and executing the build with the platform args substituted in the spec. | |
// It takes care of looping through each target platform and executing the build with the platform args substituted in the spec. |
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.
Fixed.
} | ||
|
||
// in certain conditions we allow input platform to be extended from base image | ||
if p.OS == "windows" && img.OS == p.OS { |
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.
The raw string for the OS is added here and also already exists in this file for the windowsAmd64
initialization. Should we make this a constant?
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.
This is all copied code, I'd rather not modify what doesn't need to be.
if err := json.Unmarshal(dt, &img); err != nil { | ||
return nil, nil, errors.Wrap(err, "error unmarshalling base image config") | ||
if err := json.Unmarshal(cfgs[idx], &img); err != nil { | ||
return nil, nil, nil, errors.Wrap(err, "error unmarshalling base image config") |
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.
Are this message and line 245 both true in that they're both unmarshalling base image config
?
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.
Yes, these are both true.
We take the config from the base image and then modify for whatever is in the dalec spec.
This is also returning the original base image config which gets annotated in the image metadata.
0693f94
to
22a7e3c
Compare
q: why you decided to go for |
Good question. Mostly because I wasn't sure how this would look and trying to decide if it is truly needed in the spec. |
That said, I'd like to get the underlying functionality in first, I think, because adding support in the spec is going to be a much broader change. |
This makes it so frontends can have their own args that the dalec core does not need to know about. This also moves the custom args for windowscross local to that implementation. Signed-off-by: Brian Goff <[email protected]>
This allows the builder to specify a file (either in the main build context or a custom one) which contains a list of base images that the windowscross/container target should produce images for. 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 <[email protected]>
22a7e3c
to
f5f9965
Compare
This allows builders to specify a file that contains a list of base images that the windowscross/container target should produce images for.
The result is a single multiplatform image: https://oci.dag.dev/?image=docker.io/cpuguy83/test@sha256:d88cf477b7f2464d4581f9490a8c1946300093461837272537e83879105f55e6&mt=application%2Fvnd.oci.image.index.v1%2Bjson&size=1693