Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

Introduce CLI and logging library #7

Merged
merged 8 commits into from
Jan 7, 2022
Merged

Introduce CLI and logging library #7

merged 8 commits into from
Jan 7, 2022

Conversation

ccremer
Copy link
Contributor

@ccremer ccremer commented Jan 6, 2022

Summary

  • Introduces CLI library to help getting started with flags and other configs
  • Introduces Logging library

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.

@ccremer ccremer added the enhancement New feature or request label Jan 6, 2022
@ccremer ccremer requested review from bastjan and glrf January 6, 2022 11:29
@ccremer
Copy link
Contributor Author

ccremer commented Jan 6, 2022

The default help text looks something like

NAME:
   go-bootstrap - a generic bootstrapping project

USAGE:
   main [global options] command [command options] [arguments...]

VERSION:
   unknown

COMMANDS:
   example  Start example command
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --debug, --verbose, -d  sets the log level to debug (default: false) [$BOOTSTRAP_DEBUG]
   --help, -h              show help (default: false)
   --version, -v           print the version (default: false)

Running subcommand with go run main.go example --flag bar yields

2022-01-06T12:22:56.976+0100	INFO	go-bootstrap	Starting up go-bootstrap	{"version": "unknown", "date": "2022-01-06", "commit": "-dirty-", "go_os": "linux", "go_arch": "amd64", "go_version": "go1.17.5", "uid": 1000, "gid": 1000}
2022-01-06T12:22:56.976+0100	INFO	go-bootstrap.example	Hello from example command!	{"config": {"ExampleFlag":"bar"}}

Copy link

@glrf glrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A bit over-engineered for my taste. I don't think most controllers and similar need a full blown CLI library, but 🤷

My 2 cents, but nothing I have too strong opinions on:

  • I'm not a huge fan of using cmd/ directory as anything else then containing main entry-points. I know multiple CLI libraries suggest that but it still confuses me when I see it
  • I'm not a fan of the global config variable.
  • Pretty much all K8s projects use cobra and we'll most likely have it in our transitive dependencies anyway. I know a lot of people somehow don't like cobra, but are there good reasons not to use it?

@ccremer
Copy link
Contributor Author

ccremer commented Jan 6, 2022

* I'm not a huge fan of using `cmd/` directory as anything else then containing `main` entry-points. I know multiple CLI libraries suggest that but it still confuses me when I see it

Do you have an alternative structure suggestion?
Bear in mind, it's not intended to have actual business logic in cmd/, but as you correctly assumed, just the entry points. See an example in K8up, where k8up operator is the entrypoint, but actual controller logic is in other packages: https://github.com/k8up-io/k8up/blob/master/cmd/operator/main.go

* I'm not a fan of the global config variable.

Yeah, I'm not that much fan of singletons either, but here I compromised.
Is adding a Config instance to the CLI context like AppLogger(c) a viable alternative?
The config instance can then be retrived and passed around to subcommand entrypoints.

* Pretty much all K8s projects use cobra and we'll most likely have it in our transitive dependencies anyway. I know a lot of people somehow don't like cobra, but are there good reasons not to use it?

I disagree with the statement that all K8s use cobra and for me that isn't a good enough reason to use a specific lib, especially since their versions might differ anyway. I mean also a lot of K8s projects use either klog or zap so they're already not even using the same logging framework 🤷

@glrf
Copy link

glrf commented Jan 6, 2022

Do you have an alternative structure suggestion? Bear in mind, it's not intended to have actual business logic in cmd/, but as you correctly assumed, just the entry points. See an example in K8up, where k8up operator is the entrypoint, but actual controller logic is in other packages: https://github.com/k8up-io/k8up/blob/master/cmd/operator/main.go

I personally find the k8up example even more confusing. It's called main but it's actually not a main package. I personally like to keep the hierarchy flat. So I would probably keep the command structs in the main package and not over complicate it. But calling it command instead of cmd would help me for example

Yeah, I'm not that much fan of singletons either, but here I compromised. Is adding a Config instance to the CLI context like AppLogger(c) a viable alternative? The config instance can then be retrived and passed around to subcommand entrypoints.

What I personally often do (and copied from kubectl) is to define the actual executing function (So in this case main(c *cli.Context) error) as a method on an option struct. So basically:

type exampleOption struct {
  ExampleFlag string
}

func NewExampleCommand() *cli.Command {
  o := exampleOption{}
  Command = &cli.Command{
     Name:   "example",
     Usage:  "Start example command",
    Action: o.main,
    Flags: []cli.Flag{
	&cli.StringFlag{Destination: &o.ExampleFlag, Name: "flag", EnvVars: []string{cfg.Env("EXAMPLE_FLAG")}, Value: "foo", Usage: "an demonstration how to configure the subcommand"},
    },
}

func (o exampleOption) main(c *cli.Context) error {
...
}

@ccremer
Copy link
Contributor Author

ccremer commented Jan 6, 2022

I think I can come up with something like this 👍

@ccremer
Copy link
Contributor Author

ccremer commented Jan 6, 2022

with json log format it looks like this:

go run ./... --log-format json -d example --flag "asdf"
{"level":"info","ts":1641483801.989945,"logger":"go-bootstrap","caller":"go-bootstrap/main.go:98","msg":"Starting up go-bootstrap","version":"unknown","date":"2022-01-06","commit":"-dirty-","go_os":"linux","go_arch":"amd64","go_version":"go1.17.5","uid":1000,"gid":1000}
{"level":"debug","ts":1641483801.989997,"logger":"go-bootstrap.example","caller":"go-bootstrap/example_command.go:31","msg":"validating config","version":"unknown"}
{"level":"info","ts":1641483801.9900086,"logger":"go-bootstrap.example","caller":"go-bootstrap/example_command.go:40","msg":"Hello from example command!","version":"unknown","config":{"ExampleFlag":"asdf"}}

Copy link

@glrf glrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I really like how you handle configs now and this makes commands easy to test.

@ccremer ccremer force-pushed the cli branch 2 times, most recently from 3d45c16 to a5dcfe8 Compare January 6, 2022 17:26
@ccremer ccremer force-pushed the cli branch 3 times, most recently from 884a31c to e4e3a17 Compare January 6, 2022 18:59
Copy link

@bastjan bastjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall 🚀 One question for my understanding...

Copy link

@bastjan bastjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 👍

The sample could include a a WaitGroup to make it more real life. Usually you have to wait for the shutdown.

@ccremer
Copy link
Contributor Author

ccremer commented Jan 7, 2022

@bastjan Hopefully the last objections are now integrated :)
I feel pretty confident now

Thread-safe retrieval of logging instance
Copy link

@bastjan bastjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now! 🎉 Thanks for the work 😊

@ccremer ccremer merged commit 938be08 into master Jan 7, 2022
@ccremer ccremer deleted the cli branch January 7, 2022 14:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants