Fix crash with targets show

[doanac]

fioctl targets show "" causes the tool to crash. This helps it fail more cleanly.

[doanac]

discovered by a customer

[vkhoroz]

I am still puzzled about why cobra did not catch this.
Does it mean that a dozen of other commands can fail on this too?

[doanac]

I think we probably have to set minlen for string args that require it

…

On Fri, Oct 6, 2023, 5:59 PM vkhoroz ***@***.***> wrote: ***@***.**** approved this pull request. I am still puzzled about why cobra did not catch this. Does it mean that a dozen of other commands can fail on this too? — Reply to this email directly, view it on GitHub <#303 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXSJAZDQQE6A6QMIKJSETX6CEOJAVCNFSM6AAAAAA5WLF4H6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRSHA4TAOJTHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[vkhoroz]

@doanac I'm kind of fine with this as a quick fix for the exact problem.

Still, I'm a bit of worried about the generic use case: that we might end up fixing a few dozen of places like this here and there.
So, I was curious if we can apply some generic logic somewhere.

TL;DR
The first idea that came to my mind is: that we literally never expect a user to pass us arguments like "" or '', or ever " ".
So, based on that assumption, I'd like us to have some generic check which iterates over all arguments and prevents that junk from entering.
Unfortunately, Cobra has no such attribute as DenyEmptyArgs or similar.
Fortunately, it has PersistentPreRun & PersistentPreRunE, which we can use to our benefit.

The next idea is to put that check somewhere in the root command: https://github.com/foundriesio/fioctl/blob/main/cmd/root.go#L42 like below:

var rootCmd = &cobra.Command{
    ...
    PersistentPreRunE: func (cmd *cobra.Command, args []string) error {
        for pos, val := range args {
            if len(strings.Trim(val)) == 0 {
                // You can also use PersistentPreRun and subcommand.DieNotNil(...) here
                return fmt.Errorf("Empty values or values containing only white space are not allowed for positional argument at position %d", pos)
            }
        }
        return nil
    },

This way, every single command is covered by this check uniformly, and we don't need to worry about empty values anymore.
What do you think?

[doanac]

@vkhoroz works for me. I'm not going to have a lot of time to hack this week. you want to close this out and push your own fix?

[vkhoroz]

@doanac ok, let me try to craft smth by EOW. Then I will close this PR.

[vkhoroz]

Closing this in favor of a more generic fix to all commands: #312