Skip to content
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

Fixing jf dockerscan command panic #154

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

dortam888
Copy link
Contributor

@dortam888 dortam888 commented Aug 24, 2024

  • The pull request is targeting the dev branch.
  • The code has been validated to compile successfully by running go vet ./....
  • The code has been formatted properly using go fmt ./....
  • All static analysis checks passed.
  • All tests have passed. If this feature is not already covered by the tests, new tests have been added.
  • All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

the JFrog cli might offer jf dockerscan as a command as its appearing on the command list but it has no Action function.
Added an action function to return the same output as jf docker scan would return.
When running jf dockerscan:
jf dockerscan
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x1048cbc40]

goroutine 1 [running]:
github.com/jfrog/jfrog-cli-core/v2/plugins/components.convertCommand.getActionFunc.func2(0x14000341718?)
github.com/jfrog/jfrog-cli-core/[email protected]/plugins/components/conversionlayer.go:374 +0x50
github.com/urfave/cli.HandleAction({0x105275ea0?, 0x14000419110?}, 0xa?)
github.com/urfave/[email protected]/app.go:522 +0xd8
github.com/urfave/cli.Command.Run({{0x104e7826d, 0xa}, {0x0, 0x0}, {0x0, 0x0, 0x0}, {0x104f08f05, 0x39}, {0x1400049eba0, ...}, ...}, ...)
github.com/urfave/[email protected]/command.go:175 +0x530
github.com/urfave/cli.(*App).Run(0x140003b41c0, {0x140000300a0, 0x2, 0x2})
github.com/urfave/[email protected]/app.go:277 +0x7ec
main.execMain()
github.com/jfrog/jfrog-cli/main.go:132 +0x454
main.main()
github.com/jfrog/jfrog-cli/main.go:69 +0x20

Fixing it by making it a n help command.

with: jfrog/jfrog-cli#2726

@dortam888 dortam888 added bug Something isn't working safe to test Approve running integration tests on a pull request labels Aug 24, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Aug 24, 2024
Copy link
Contributor

@attiasas attiasas left a comment

Choose a reason for hiding this comment

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

what is the panic? please add to the description.
I could not replicate the issue.
Also this is a hidden command that inject and shows the HELP for jf docker scan it is not a command on its own.

@dortam888
Copy link
Contributor Author

@attiasas When running jf dockerscan even if by accident we're getting panic instead of handling.
We have many hidden commands but with action function.
Either jf dockerscan will do the same as jf docker scan (that's what I implemented)
Or we should output something like:
'jf dockerscan' is not a jf command. See --help
The most similar command is:
jf docker scan

Take in account that if we're running something close to jf dockerscan it will output:
'jf dockersca' is not a jf command. See --help
The most similar command is:
jf dockerscan
And I can't change it since its reading it as a command

@attiasas
Copy link
Contributor

attiasas commented Sep 4, 2024

@dortam888 , As discussed, this needs to be behave similar to dockerpullhelp in the CLI project

Description: "The docker image tag to scan.",
},
}
func GetArguments() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

return this as []components.Argument as we do so for all other commands. we don't want to create a different types just for one single command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants