-
Notifications
You must be signed in to change notification settings - Fork 558
commands: remove debug package in commands #3240
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request removes the legacy debug package from the commands and replaces its usage with a new debugging command implementation. Key changes include:
- Removing the debug package code (commands/debug/root.go) entirely.
- Updating commands/root.go to use a new debugging command.
- Refactoring commands/build.go to accept the revised debug options type.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
commands/root.go | Updated to remove debug package dependency and call a new debugging command |
commands/debug/root.go | Removed the legacy debug command implementation |
commands/debug.go | Introduced a new debug command using debugOptions |
commands/build.go | Refactored buildCmd to use debugOptions instead of debug.DebugConfig |
Comments suppressed due to low confidence (1)
commands/root.go:122
- The function 'dapCmd' is not defined in the diff and appears to be a typo. It is likely that 'debugCmd' (defined in commands/debug.go) was intended here.
cmd.AddCommand(dapCmd(dockerCli, opts))
2cf3ce3
to
23788a0
Compare
The copilot comment does not really seem to be correct. There's no legacy debug command or implementation. I just copied it to the same package as the other commands. I didn't want someone to get the wrong impression from it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs needs regeneration
The package just causes the entire flow to be more complicated as build has to pretend it doesn't know about debug options and the debugger has to pretend it doesn't know about the build. This abstraction has been difficult when integrating a DAP command into this same workflow so I don't think this abstraction has much of a value. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
23788a0
to
7f5ff6b
Compare
The docs change was caused by removing the |
The package just causes the entire flow to be more complicated as build
has to pretend it doesn't know about debug options and the debugger has
to pretend it doesn't know about the build.
This abstraction has been difficult when integrating a DAP command into
this same workflow so I don't think this abstraction has much of a
value.