datreeio / datree

Prevent Kubernetes misconfigurations from reaching production (again 😤 )! From code to cloud, Datree provides an E2E policy enforcement solution to run automatic checks for rule violations. See our docs: https://hub.datree.io
https://datree.io
Apache License 2.0
6.39k stars 363 forks source link

Print version messages using Cobra PreRun Hook #386

Closed noaabarki closed 2 years ago

noaabarki commented 2 years ago

Describe the solution you'd like

Use Cobra Hooks before the command to check for version messages.

Resources: https://umarcor.github.io/cobra/#prerun-and-postrun-hooks

”How to implement” Suggestion We print version messages in the same way on every command. For instance, you can find the logic in datree test inside evaluate function located in cmd/test/main.go (look for LoadVersionMessages 😬).

Eventually, adding the logic with PreRun hook will look more or less like this:

&cobra.Command{
        PreRunE: func(cmd *cobra.Command, args []string) error {
            // the logic
        },
        ...
        RunE: func(cmd *cobra.Command, args []string) error {...},
    }

Requirements Golang basic level.

amustaque97 commented 2 years ago

I would love to take it up! Just to confirm again. We need to refactor LoadVersionMessages from all cmds and write it once under PreRunE hook!

noaabarki commented 2 years ago

Hey @amustaque97! Awesome, you got it and yes, you are correct.

amustaque97 commented 2 years ago

@noaabarki, may I know what are we trying to achieve here?

Before jumping into implementation I want to discuss concerns https://github.com/datreeio/datree/blob/main/cmd/test/main.go#L277-L288

Currently in the above link we are calling LoadVersionMessages only in case of InteractiveMode and If we move this under pre hook it will work but then again in the evaluate function we need to check InteractiveMode.

We cannot remove InteractiveMode as it will hamper other section of codebase. OR in PreRunE hook irrespective of mode will call LoadVersionMessages.

What do you think?

noaabarki commented 2 years ago

Hey @amustaque97!! I hope I'm not missing something. We want to print the version messages on every command execution. The messages should be printed first. By separating the logic for printing the version messages and moving them to outside the run command we avoid redundant code while keeping the same behavior for all commands.

Regarding non-interactive-mode executions - you were right to bring that up. We'd like to print the messages as well however they should be printed in the requested output form.

What do you think?

amustaque97 commented 2 years ago

Thanks for the reply! Yeah, I agree with you 💯 . Will make changes and raise a PR 🤓 .