Skip to content

Conversation

@ktock
Copy link
Collaborator

@ktock ktock commented Apr 26, 2023

Following-up #1640 (comment)

From @jedevc 's comment:

The commands in the monitor are getting long - we really need to split these up, out of a switch statement, and ideally into
something a bit more dynamic. Something like implementations of a monitor.Command interface?

As inspiration:

type Command interface {
    // executes the command
    Exec(args []string /* anything else we might need*/)

    // returns command information
    Info() Info
}

type Info struct {
    // a usage string, e.g. "rollback [--init] [entrypoint] [cmd...]"
    Usage string

    // a short description, e.g. "re-runs the interactive container with initial rootfs contents"
    Description string

    // a longer description, describes in detail what the command does, the
    // behavior of each arg, etc.
    Help string
}

This commit moves monitor commands to monior/commands package. Commands still need access to the monitor object and buildx controller so this commit enables this via Monitor interface stored in monitor/types.

Copy link
Collaborator

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

Just a couple of initial thoughts, haven't done a proper deep dive into the command implementations yet, will try and get to that tomorrow ❤️

@jedevc
Copy link
Collaborator

jedevc commented Apr 26, 2023

Could you rebase onto master since #1737 merged?

@ktock ktock force-pushed the monitor-commands branch from 330b577 to 0227ce0 Compare April 27, 2023 02:19
ktock added 2 commits April 27, 2023 11:36
This commit moves monitor commands to `monior/commands` package.
Commands still need access to the `monitor` object and buildx controller so this
commit enables this via `Monitor` interface stored in `monitor/types`.

Signed-off-by: Kohei Tokunaga <[email protected]>
Signed-off-by: Kohei Tokunaga <[email protected]>
@ktock ktock force-pushed the monitor-commands branch from 0227ce0 to 9f884ed Compare April 27, 2023 02:38
Copy link
Collaborator

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

This is good now IMO 🎉

I think we can do some future tidy-ups, but this is a significant improvement from the big switch statement 🎉 🎉

Thanks!

@jedevc jedevc merged commit c7c37c3 into docker:master May 3, 2023
@ktock ktock deleted the monitor-commands branch May 3, 2023 13:36
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.

2 participants