Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cpuguy83
Copy link
Member

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

@cpuguy83 cpuguy83 requested a review from a team as a code owner December 18, 2024 00:47
Copy link

@MorrisLaw MorrisLaw left a 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)

@@ -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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit:

Suggested change
// 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.

Copy link
Member Author

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 {

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?

Copy link
Member Author

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")

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?

Copy link
Member Author

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.

@DannyBrito
Copy link
Contributor

q: why you decided to go for refs to be build-arg instead of part of the spec?

@cpuguy83
Copy link
Member Author

q: why you decided to go for refs to be build-arg instead of part of the spec?

Good question. Mostly because I wasn't sure how this would look and trying to decide if it is truly needed in the spec.
After some more thought I do have an idea of how to do this and will be a nice replacement for the image.base.

@cpuguy83
Copy link
Member Author

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants