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

Use controller option in bake #1680

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Mar 15, 2023

🗒️ Marking as draft until we merge #1671.

This PR continues the move to attempt to merge the build.Options struct into the controllerapi.Options message.

To do this, we extract all the input parameters into a dedicated message (adding the missing ones, except for the InStream parameter which will require some additional fiddling around). We also rework the NamedContexts to allow containing States (by transmitting them as DefinitionOps), and adding a Linked field to the common options.

With these changes, we can have bake generate controllerapi.BuildOptions, and then convert those to build.Options using the controller/build package.

This is an intermediate patch, designed to allow us to clean up some shared logic between both build and bake. The next step will be to modify bake to use the controller API directly, and completely skip the build.Options generation step.

Copy link
Collaborator

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Thank you for working on it.

controller/build/build.go Show resolved Hide resolved
@jedevc jedevc force-pushed the bake-use-controller-options branch from c550e4c to 6718ee5 Compare March 15, 2023 13:04
@ktock
Copy link
Collaborator

ktock commented Mar 15, 2023

Thanks, LGTM.

@@ -118,6 +131,7 @@ message CommonOptions {
bool Pull = 4;
bool ExportPush = 5;
bool ExportLoad = 6;
bool Linked = 7;
Copy link
Member

Choose a reason for hiding this comment

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

Linked is internal. I'm a bit on the fence to have it in the proto def:

buildx/build/build.go

Lines 93 to 94 in 4a73abf

// Linked marks this target as exclusively linked (not requested by the user).
Linked bool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - but based on the current usage of it, I think we need to. It's used in BuildWithResultHandler which will be called from the controller side, while it's generated from targets before we make the controller request.

I think it's only used here?

buildx/build/build.go

Lines 688 to 690 in 4a73abf

if !opt.Linked && len(opt.Exports) == 0 {
noOutputTargets = append(noOutputTargets, name)
}

I'm quite sure how else we can do it, though open to suggestions 🤔

Copy link
Member

@crazy-max crazy-max Mar 15, 2023

Choose a reason for hiding this comment

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

Ah right let's keep then 👍 (with a comment in the proto)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I think we could get rid of it, if we could move the default outputs onto the client side of the controller (which would probably be a better design anyways).

Might be slightly tricky, since we only load the driver on the controller server (which we probably want to change anyways), so maybe worth returning to this with that refactor?

@jedevc jedevc force-pushed the bake-use-controller-options branch from 6718ee5 to b5ab2a8 Compare March 15, 2023 14:03
This patch continues the move to attempt to merge the build.Options
struct into the controllerapi.Options message.

To do this, we extract all the input parameters into a dedicated message
(adding the missing ones, except for the InStream parameter which will
require some additional fiddling around). We also rework the
NamedContexts to allow containing States (by transmitting them as
DefinitionOps), and adding a Linked field to the common options.

Signed-off-by: Justin Chadwell <[email protected]>
With the previous changes to bring controllerapi.BuildOptions up to date
with build.Options, we can have bake generate
controllerapi.BuildOptions, and then convert those to build.Option using
the controller/build package.

This is an intermediate patch, designed to allow us to clean up some
shared logic between both build and bake. The next step will be to
modify bake to use the controller api, and completely skip the
build.Options generation step.

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc force-pushed the bake-use-controller-options branch from b5ab2a8 to d4e38a0 Compare March 15, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants