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

Specific account names can lead to the wrong error occuring #147

Closed
JanLukasGithub opened this issue Jun 3, 2022 · 14 comments · Fixed by #149
Closed

Specific account names can lead to the wrong error occuring #147

JanLukasGithub opened this issue Jun 3, 2022 · 14 comments · Fixed by #149
Labels
bug Something isn't working

Comments

@JanLukasGithub
Copy link
Contributor

JanLukasGithub commented Jun 3, 2022

When you call gscloud server create --account make-config --name testName --cores 1 --mem 1, it doesn't matter if there's an account make-config in the config file, and the program just attempts to create a server which will fails due to no authorization, because the account doesn't exist and no UUID/token were loaded

The other account names this applies to are version, manpage and completion

Similar thing happens with gscloud server create --name completion --cores 1 --mem 1 when no config exists, and completion may be a name you want to give your server

Expected result would be this error account 'make-config' does not exist

@JanLukasGithub
Copy link
Contributor Author

So what happens is that CommandWithoutConfig only checks if any argument is make-config, version, manpage or completion, but disregards if it is the command called or a flag like --account. I have a test for this already, but don't know how to properly implement it due to lack of experience with cobra

@bkircher bkircher added the bug Something isn't working label Jun 3, 2022
@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 3, 2022

Problems could also arise when renaming the executable to one of the mentioned names, as os.Args[0] is the program name

@bkircher
Copy link
Contributor

bkircher commented Jun 3, 2022

Indeed. CommandWithoutConfig is called with os.Args everywhere. It's not a good implementation. A better way would be to check the current Cobra sub-command name if it's in that list instead of reading os.Args.

@JanLukasGithub
Copy link
Contributor Author

But Cobra only let's you check if there's a sub-command afaik

@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 3, 2022

This works but isn't really well maintainable

This was referenced Jun 3, 2022
@JanLukasGithub
Copy link
Contributor Author

But Cobra only let's you check if there's a sub-command afaik

you can do rootCmd.Commands() to get sub-commands (I want to kill the person who thought let's call sub-commands Commands now), but I still haven't found out how to check if the Command has been called

@nvthongswansea
Copy link
Member

I believe the sub-command's name is always the 1st string in the list without the flag notation --. If so, just iterate over the list and stop when the 1st str without --, this str will be the sub-command. Correct me if I'm wrong.

@nvthongswansea
Copy link
Member

btw, flags can be shorthand flags -.

@nvthongswansea
Copy link
Member

So CommandWithoutConfig will be sth like:

func CommandWithoutConfig(cmdLine []string) bool {
	var noConfigNeeded = []string{
		"make-config", "version", "manpage", "completion",
	}
	for _, arg := range cmdLine {
                if !strings.HasPrefix(arg, "-") {
                   // do the check and break the loop.
                } 
	}
	return false
}

@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 3, 2022

I believe the sub-command's name is always the 1st string in the list without the flag notation --. If so, just iterate over the list and stop when the 1st str without --, this str will be the sub-command. Correct me if I'm wrong.

Flags can have different notations like --flag value, --flag=value or -f value, which is exactly why Cobra is used I guess, and you don't want to have two different parsing functions ideally, but because Cobra apparently doesn't support this use-case it's unfortunately needed. I think I'll open an issue on their Github if I do not find a way to do this

@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 3, 2022

I think this can be done with rootCmd.Find() somehow but I don't have the time to continue testing how exactly, as calling the function finds some sub-commands (make-config, manpage, completion) but not others (version)

@ghostwheel42
Copy link
Contributor

Hi. You can use PersistentPreRunE to check & load the configuration only when needed similar to this code fragment:

	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
		// check config file
		if !commandWithoutConfig(cmd.Name()) {
			if err := viper.ReadInConfig(); err != nil {
				return err
			}
		}
		return nil
	},

A noConfigNeeded[]string can be initialized with []string{"help", "completion"} and filled in each subcommands init() function using a call to runsWithoutConfig():

// runsWithoutConfig registers a command to run without a configuration file.
func runsWithoutConfig(cmds ...*cobra.Command) {
	for _, cmd := range cmds {
		noConfigNeeded = append(noConfigNeeded, cmd.Use)
	}
}

The check would be this:

// commandWithoutConfig tells if command needs a config file.
func commandWithoutConfig(command string) bool {
	for _, check := range noConfigNeeded {
		if command == check {
			return true
		}
	}
	return false
}

@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 3, 2022

This is a major redesign though and would need rather extensive testing...

If there's an easier way to modify the current approach I'd prefer that

@JanLukasGithub
Copy link
Contributor Author

JanLukasGithub commented Jun 7, 2022

I think this can be done with rootCmd.Find() somehow but I don't have the time to continue testing how exactly, as calling the function finds some sub-commands (make-config, manpage, completion) but not others (version)

So I found out, that renaming root.go to zroot.go (so it's sorted as last command) allows you to use rootCmd.Find(os.Args[1:]) to get the name of the sub-command that'll be used

This only has the problem that no sub-command can have further sub-commands called version, manpage, make-config or completion

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 a pull request may close this issue.

4 participants