Skip to content

cli/debug: move to "cmd/dockerd/debug"#47981

Merged
thaJeztah merged 1 commit intomoby:masterfrom
gtomitsuka:move-debug-to-dockerd
Jun 14, 2024
Merged

cli/debug: move to "cmd/dockerd/debug"#47981
thaJeztah merged 1 commit intomoby:masterfrom
gtomitsuka:move-debug-to-dockerd

Conversation

@gtomitsuka
Copy link
Contributor

@gtomitsuka gtomitsuka commented Jun 13, 2024

Description
The "cli/debug" package is a leftover from when the "docker" and "dockerd" cli were both maintained in this repository, and finding a more appropriate home for it has been a roadmap task since "docker/cli" was moved to its own repo – see #44641.

Currently, the primary user is "cmd/dockerd". Accordingly, I made it a subpackage of "cmd/dockerd" – "cmd/dockerd/debug".
In particular, I tried to match styles with "cmd/dockerd/trap", the other subpackage of "cmd/dockerd". Both packages are also imported by the "daemon" package.

Since it's binary-related and not library code, this shouldn't impact any external packages (all known importers on pkg.go.dev are moby forks).

- Description for the changelog

Move the cli/debug package to cmd/dockerd/debug

- A picture of a cute animal (not mandatory but encouraged)

It's Juicy's world, we're just living in it.
PHOTO-2022-09-23-16-00-10

Signed-off-by: Gabriel Tomitsuka <gabriel@tomitsuka.com>
@thaJeztah thaJeztah added area/cli Client status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 13, 2024
@thaJeztah thaJeztah added this to the 27.0.0 milestone Jun 13, 2024
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 076c976 into moby:master Jun 14, 2024
@thaJeztah
Copy link
Member

@gtomitsuka Thank you!! If you're interested; I once did a very brief attempt at moving the remaining cli/winresources package to their individual cmd/xxx sub packages. I was hoping that to be not too complicated, but it looks like there were still some things to untangle (so that still required work)

My draft PR with that first attempt is #46886 (I just rebased it); feel free to take that branch if you're interested on working on that (and if my branch is useful 😄).

@gtomitsuka
Copy link
Contributor Author

@thaJeztah thanks for rebasing, I'll look into it this weekend!

@thaJeztah
Copy link
Member

Thanks! And don't feel "required" to do so, but it's just one of those patches that are in the back of my head and "I need to spend some time making it work". In all honesty, I'm not too familiar with that code, so I would have to do some digging (and your guess is probably as good as mine!).

I can list some quick thoughts;

  • Goal is to get rid of that CLI package (as it's really no longer needed)
  • With the ongoing effort of (one day!) being an actual go module, we want to better organise the code (separate things we expect to be consumed from "binary" code - only used to build the engine itself); possibly this could involve multiple modules, but those can be a bit of a pain, so that's still somewhat "TBD"
  • For the PR linked above; if that means duplicating some files (icon files, manifest-templates) for better separation; that's perfectly acceptable
  • If moving the code works; that's good, but we're also not "tied" to the implementation; ISTR the CLI uses a different utility for the winresources; if that's indeed the case, and if that implementation is better to use, we can consider switching (but that could also be in a follow-up after); https://github.com/docker/cli/tree/925e7d687045ea0c5a674c830762b7787c3253b1/cli/winresources

@crazy-max did some of the work on these, and if needed may be able to provide more input 🙈 (sorry for the ping, Kevin!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli Client kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

3 participants