Skip to content

Refactor controllers, moving them into a separate package#1579

Merged
jedevc merged 2 commits into
docker:masterfrom
jedevc:refactor-controllers
Feb 1, 2023
Merged

Refactor controllers, moving them into a separate package#1579
jedevc merged 2 commits into
docker:masterfrom
jedevc:refactor-controllers

Conversation

@jedevc
Copy link
Copy Markdown
Collaborator

@jedevc jedevc commented Jan 31, 2023

⬆️ Follow up-to #1296

We should refactor out the different controllers (local and remote) into a separate package. I tried having a look at this myself, but as is, there's some strong coupling between the cli and the controllers. Even if that gets nicely separated out, we end up in a scenario where build and bake still use the same utilities, which makes things tricky. This ended up being kind of a time sync, and IMO, we should probably first work out how we want this design to interact with bake before going too much further.
I'm happy to take a look at this once we merge, I already spent some time getting familiar with what will make this tricky.

Looks like this was less tricky than imagined 🎉

The core component of this PR is the creation of a new top-level package controller:

  • The top-level has a single function NewController to create a controller.
  • control contains the interfaces and options definitions for controllers.
  • local contains the code for the Local buildx controller (running in-process)
  • remote contains the code for the Remote buildx controller (running out-of-process)
  • build contains the high level logic to run the build - eventually at some point, it's likely this will probably only be called from within the controller package. However, for now, we only use the local and remote controllers when in experimental mode, so we need to continue to properly expose this.
    There may be a better place for this? Not sure, but now that it's split out it should be very easy to move.

Ideally we can do this before too much more feature work, but it would be good to get an API surface that works.

@jedevc jedevc force-pushed the refactor-controllers branch 2 times, most recently from e9c2a87 to 9c91a77 Compare January 31, 2023 17:34
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the refactor-controllers branch from 9c91a77 to 5f130b2 Compare January 31, 2023 17:51
Copy link
Copy Markdown
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!

@jedevc jedevc merged commit 6506166 into docker:master Feb 1, 2023
@jedevc jedevc deleted the refactor-controllers branch February 1, 2023 09:57
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.

3 participants