opiproject / sztp

Secure Zero Touch Provisioning (sZTP) in OPI
Apache License 2.0
20 stars 14 forks source link

Implement easy cobra commands, fix command outputs #413

Closed Xeckt closed 4 months ago

Xeckt commented 4 months ago

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.

```go
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.

glimchb commented 4 months ago

make sense, @Xeckt go for it!

glimchb commented 4 months ago

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 commented 4 months ago

Cool, I'll get started!

Xeckt commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.