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

Implement easy cobra commands, fix command outputs #413

Closed
Xeckt opened this issue Jun 24, 2024 · 6 comments · Fixed by #424
Closed

Implement easy cobra commands, fix command outputs #413

Xeckt opened this issue Jun 24, 2024 · 6 comments · Fixed by #424
Assignees

Comments

@Xeckt
Copy link
Contributor

Xeckt commented Jun 24, 2024

At the moment, the source utilises Cobra CLI in a simple form when it comes to adding the commands to it. In the main file for the sztp-agent you have the following:

https://github.com/opiproject/sztp/blob/33b5b5ce949c76c17786e71e9dc933443b294e15/sztp-agent/main.go#L28C1-L47C2

The individual use of c.AddCommand(...) can be quite cumbersome in the future where the CLI grows. So I'd like to propose a solution for this:

  1. Create a global var Commands []*cobra.Command slice
  2. Have all the cobra commands setup inside an init() func instead of NewXCommand
  3. All the init() funcs for all commands append themselves to the commands slice
  4. Loop through the slice in main:
for _, cmd := range Commands {
   c.AddCommand(cmd)
}

Additionally, there seems to be something I think could be changed. Every time a command might fail, it runs prints the help text. I was testing it with my new command I created for generating systemd unit files:

~/GolandProjects/sztp/sztp-agent git:[main]
./sztp-agent systemd
Error: creating unit file /etc/systemd/system/sztp-agent.service: open /etc/systemd/system/sztp-agent.service: permission denied
Usage:
  opi-sztp-agent systemd [flags]

Flags:
  -h, --help             help for systemd
  -o, --options string   sztp-agent args/flags to add into the unit file
  -p, --path string      Path for unit file to be created (default "/etc/systemd/system")

2024/06/24 16:06:34 [ERROR] creating unit file /etc/systemd/system/sztp-agent.service: open /etc/systemd/system/sztp-agent.service: permission denied

And it prints the error twice, same with the other commands created. In my personal experience this should only happen if the user specifically specifies --help | -h with a command and/or subcommand. Not upon command failure, otherwise it drowns out the real error message.

I believe the structure of cobra needs some amendments and fixes to streamline it and make it cleaner. E.g.

func main() {
	command := newCommand()
	if err := command.Execute(); err != nil {
		log.Fatalf(color.InRed("[ERROR] ")+"%s", err.Error())
	}
}

func newCommand() *cobra.Command {
	c := &cobra.Command{
		Use:   "opi-sztp-agent",
		Short: "opi-sztp-agent is the agent command line interface to work with the sztp workflow",
		Run: func(cmd *cobra.Command, _ []string) {
			var err error
			if err != nil {
				log.Fatalf(color.InRed("[ERROR] ")+"%s", err.Error())
			}
		},
	}

	c.AddCommand(cmd.NewSystemdCommand())
	c.AddCommand(cmd.NewEnableCommand())
	return c
}

This can be done all under main(), and my proposed changes to how we add commands.

I'd be happy to implement this and make all the necessary changes. This way, we'd never have to do anything with the main function unless necessary, and adding commands becomes a little bit more straightforward.

@Xeckt Xeckt changed the title Implement easy cobra commands Implement easy cobra commands, fix command outputs Jun 24, 2024
@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

make sense, @Xeckt go for it!

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

couple more things that could help:

  1. status command has wrong arguments
  2. run and daemon should do exactly the same, but daemon in the loop and run only once

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Cool, I'll get started!

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

couple more things that could help:

  1. status command has wrong arguments
  2. run and daemon should do exactly the same, but daemon in the loop and run only once

Could you abbreviate more on the status command? It looks like it's templated from another command file but I suppose it's function is different. Judging by the name it reports the status, can you give me more details on that?

Does the daemon need to run in the background or it can take the current session over?

@glimchb
Copy link
Member

glimchb commented Jun 24, 2024

I opened separate issue for status with much more details here #399
daemon like today can take over the session but in loop until success
run is like daemon but exits after one iteration with success or fail

@Xeckt
Copy link
Contributor Author

Xeckt commented Jun 24, 2024

Ok, I will make the changes for the CLI as mentioned in this issue and make a PR, then I'll handle that issue seperately.

@Xeckt Xeckt mentioned this issue Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants