🐛 exit after showing help #468
Conversation
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
There was a problem hiding this comment.
@Vickysomtee hey thank you for your fix. it should work! But I noticed that when we integrated cobra, we didn't do things quite the "cobra" way. Are you willing to help us refactor that code? The changes I am looking for are -
- main() function only calls cmd.Execute() The actual code that runs the analysis is removed from main()
- analysis code goes into command's RunE() funcion instead
Here's a good example of using different hooks in a cobra command: https://github.com/konveyor/kantra/blob/20af46fafa3876cd56f5b3be20610c7185673142/cmd/analyze.go#L85-L150
@pranavgaikwad Yes, I would love to refactor this. Few questions though
|
Yeah absolutely
https://github.com/konveyor/analyzer-lsp/blob/main/cmd/analyzer/main.go#L201 This is where the engine runs the rules, but all the setup is also very important https://github.com/konveyor/analyzer-lsp/blob/main/cmd/analyzer/main.go#L96-L199 The important parts here are rule parsing and getting and setting up the correct providers.
No test will be needed for this refactor, our e2e tests will test this! |
|
@shawn-hurley Thank you! |
|
@Vickysomtee let me know if you want to get on a call to help get the demo testing working, and continue on this! |
|
I think I might need help to get the demo test working but it's not currently the blocker for this PR. I had a little tight schedule but I am free now and I will continue on this. If I still need help on the demo test, I would let you know. Thank you! |
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
|
I have updated the PR with the refactor @pranavgaikwad @shawn-hurley |
|
@Vickysomtee Thank you. Really appreciate you helping out. Is it ready for review? |
@pranavgaikwad Yes it is |
cmd/analyzer/main.go
Outdated
|
|
||
| func main() { | ||
| if err := AnalysisCmd().Execute(); err != nil { | ||
| println(err.Error()) |
There was a problem hiding this comment.
Can we use os.Exit(1) here?
cmd/analyzer/main.go
Outdated
| rootCmd := &cobra.Command{ | ||
| Use: "analyze", | ||
| Short: "Tool for working with analyzer-lsp", | ||
| PreRun: func(c *cobra.Command, args []string) { |
There was a problem hiding this comment.
Instead of exiting here, can we use PreRunE function and only return an error?
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
@pranavgaikwad I have made the changes |
| PreRunE: func(c *cobra.Command, args []string) error { | ||
| err := validateFlags() | ||
| if err != nil { | ||
| errLog.Error(err, "failed to validate flags") |
There was a problem hiding this comment.
nit: can we return err here? and return nil after the if block? just making sure if we make changes in future, it doesn't go unnoticed that err could be non-nil after the if block
Signed-off-by: vickysomtee <vickysomtee@gmail.com>
Fixes #439