slimtoolkit / slim

Slim(toolkit): Don't change anything in your container image and minify it by up to 30x (and for compiled languages even more) making it secure too! (free and open source)
Apache License 2.0
18.97k stars 706 forks source link

added documentation of `debug` command fixes- #372 #535

Closed Omkar0114 closed 1 year ago

Omkar0114 commented 1 year ago

Fixes - #372

Signed-off-by: Omkar Kulkarni opkulkarni0104@gmail.com

Omkar0114 commented 1 year ago

Hi @kcq, @iximiuz raised a PR for debug command. Please review it and suggest any changes much appreciated :)

kcq commented 1 year ago

Thank you for the PR @Omkar0114 . It's a good start. It'll be nice to add a couple of extra enhancements.

The COMMANDS sub-section of the BASIC USAGE INFO section should mention the debug command. The USAGE DETAILS section should mention the debug command too (in its summary sub-section).

Also good to mention that the target for the debug command is a container name, which is different from container image name or ID for other commands.

And it's a good idea to document the behavior in this code: https://github.com/slimtoolkit/slim/blob/master/pkg/app/master/commands/debug/cli.go#L74

There you can pass custom container "CMD" parameters after -- (kind of like what happens with kubectl exec where you pass custom command line params after --).

Omkar0114 commented 1 year ago

Hi @kcq Thanks for the review. I made changes as you suggested in the README file but not clearly getting exactly what needs to be done in the cli.go code here https://github.com/slimtoolkit/slim/blob/master/pkg/app/master/commands/debug/cli.go#L74

I am assuming that we need to add container parameters after -- like --target, -it, or do we need to add a comment for this code? can you elaborate on this?

Thank you so much, Kyle!!

kcq commented 1 year ago

Don't worry about cli.go and --. Let's get the initial version in first. We can make additional enhancements later as a follow up.

Omkar0114 commented 1 year ago

Ok. updated PR with suggested changes. Take a look once @kcq Thanks! Suggest any other changes if needed

Omkar0114 commented 1 year ago

Don't worry about cli.go and --. Let's get the initial version in first. We can make additional enhancements later as a follow up.

We can open a new issue for this enhancement later as you said. I'll work on that as well. :)