Switch to kong (aka support reading flags from env) #51

halkeye · 2024-10-14T07:23:14Z

Kong supports env variables out of the box, and a little easier to make commands (it seems).

This really just started as an excersize to see if i could hook up env variables easily, and the more i read the more i saw kong was suggested over cobra + viper these days. and I can kinda see why.

I tried to keep the same "unifibackup --bucket name" support so by default if no other args are given, it'll run backup

UNIFIBACKUP_BUCKET=halkeye-unifi go run ./...

or

go run ./... --bucket halkeye-unifi

The only thing I couldn't keep was --help defaulting to "backup"

go run ./... backup --help
Usage: unifibackup backup --bucket=STRING [flags]

Copies UniFi Controller backups to S3

Flags:
  -h, --help                                      Show context-sensitive help.
      --version

      --dir="/var/lib/unifi/backup/autobackup"    path of the UniFi autobackup directory ($UNIFIBACKUP_DIR).
      --bucket=STRING                             name of the S3 bucket to upload to ($UNIFIBACKUP_BUCKET).
      --prefix="unifi/"                           prepended to the backup file name to form the object key ($UNIFIBACKUP_PREFIX).
      --metrics=":9184"                           listen spec for the web server that exposes Prometheus metrics ($UNIFIBACKUP_METRICS).
      --timeout=5m                                time to allow for upload and delete requests ($UNIFIBACKUP_TIMEOUT).

halkeye · 2024-10-14T07:23:52Z

Again, this was just a fun excersize, i have no objections to not being merged, i just thought it was a win and worth sharing

Kong supports env variables out of the box, and a little easier to make commands (it seems).

gebn · 2024-10-14T20:18:08Z

cmd/unifibackup/daemon.go

-	"github.com/gebn/unifibackup/v2/cmd/unifibackup/monitor"
-	"github.com/gebn/unifibackup/v2/cmd/unifibackup/uploader"
-


This is interesting, I've always grouped imports by stdlib, project packages, then other packages. Do you have a reference for swapping other and project?

oh my editor is setup to auto organize imports, i think via gopls or https://github.com/incu6us/goimports-reviser, i havn't really thought about it as long as its consistent.

gebn · 2024-10-14T20:19:47Z

cmd/unifibackup/main.go

@@ -32,73 +32,56 @@ var (
 	})
 )

+var CLI struct {


Could this be made a local variable within main(), or at least be unexported?

yea. Their getting started docs are a little rough, so i didn't realize the name didn't matter till much later.

gebn · 2024-10-14T20:25:40Z

cmd/unifibackup/main.go

+		err := genmeta(CLI.Genmeta.Dir)
+		if err != nil {


Suggested change

err := genmeta(CLI.Genmeta.Dir)

if err != nil {

if err := genmeta(CLI.Genmeta.Dir); err != nil {

gebn · 2024-10-14T20:26:32Z

cmd/unifibackup/main.go

-		Args:  cobra.NoArgs,
-		RunE: func(_ *cobra.Command, _ []string) error {
-			return genmeta(flgBackupDir)
+	ctx := kong.Parse(


ctx is conventionally reserved in Go for context.Context, and there is some shadowing going on as a result. How about kongCtx?

gebn · 2024-10-14T20:29:35Z

cmd/unifibackup/main.go

+		cfg, err := config.LoadDefaultConfig(ctx,
+			config.WithUseDualStackEndpoint(aws.DualStackEndpointStateEnabled))
+		if err != nil {
+			panic(fmt.Errorf("failed to initialise AWS SDK: %w", err))


It's a good idea not to panic(), perhaps it would be worth copying the pattern here and returning the error from this function instead?

i mean printing the error and exiting with code 1 is the same as panic
but sure, i can switch it to the same pattern

Ack, it's more about having main() be the (single) point of exit - knowing all other functions will return in all cases simplifies the mental model, and means we can choose how to handle those errors at the caller rather than callee.

yep yep, very good practice. I didn't see a setup like that with cobra so was trying to stick with what was already done.

gebn · 2024-10-14T20:30:59Z

cmd/unifibackup/main.go

+		if err != nil {
+			panic(fmt.Errorf("failed to initialize daemon: %w", err))
+		}
+		break


Go switch cases do not fall-through, so these are redundant.

gebn · 2024-10-14T20:31:55Z

cmd/unifibackup/main.go

+		}
+		break
+	default:
+		panic(ctx.Command())


Suggested change

panic(ctx.Command())

return fmt.Errorf("unknown command: %v", kongCtx.Command())

halkeye · 2024-10-14T20:40:29Z

oh cool, this sounds like its not a no. I'll try to clean it up this afternoon. That'll save me from having some weird hacking of the arguments to make all but my bucket name public (I like having my infra as code public for others to see from)

gebn · 2024-10-14T20:44:50Z

The use of env vars vs. flags shouldn't change that, but I get the former are easier to template and compose. Cobra does have 37.9k stars vs. Kong's 2.1k. They are both mature, stable libraries. The main thing I care about is not breaking people.

halkeye · 2024-10-15T07:23:37Z

he use of env vars vs. flags shouldn't change tha

in my kubernetes setup i can inject secrets as env variables but not as command arguments. I was gonna try out doing bash -c, but its inside of a scratch container, so no shell either.

so thats what got me thinking if this was doable.

gebn · 2024-10-15T09:21:57Z

You should be able to reference env vars in arguments e.g.

args: [
  "--bucket", "$(BUCKET)",
]

halkeye · 2024-10-15T16:48:00Z

What syntax is that?
are you talking about https://kubernetes.io/docs/tasks/inject-data-application/define-interdependent-environment-variables/ ?
I didn't know you could refer to other env variables but I'm surprised the env variables work in the cli
I'd have to try and build a new scratch container and try it out.

gebn · 2024-10-15T21:53:59Z

Nothing more than this.

halkeye · 2024-10-17T17:26:35Z

Nothing more than this.

thanks so much for this. I didn't realize it was an option, let alone reading from envFrom, a lot less bash -c in my future :)

Do you want to close this or merge it?
Also I have a few others I can submit as PRs:

halkeye@a453266 - publish to github packages so forks can publish too
halkeye@1a94cda - have some startup text with fields for easier debugging
halkeye@c2af442 - disable dual stack endpoints since I'm using DO not AWS (need to wait till midnight to test this one)

Switch to kong

824d1e9

Kong supports env variables out of the box, and a little easier to make commands (it seems).

halkeye force-pushed the switch-to-kong branch from a5f9a0a to 824d1e9 Compare October 14, 2024 07:55

gebn reviewed Oct 14, 2024

View reviewed changes

pr comments

ff13811

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to kong (aka support reading flags from env) #51

Switch to kong (aka support reading flags from env) #51

halkeye commented Oct 14, 2024

halkeye commented Oct 14, 2024

gebn Oct 14, 2024

halkeye Oct 14, 2024

gebn Oct 14, 2024

halkeye Oct 14, 2024

gebn Oct 14, 2024

gebn Oct 14, 2024

gebn Oct 14, 2024

halkeye Oct 14, 2024

gebn Oct 14, 2024

halkeye Oct 14, 2024

gebn Oct 14, 2024

gebn Oct 14, 2024

halkeye commented Oct 14, 2024

gebn commented Oct 14, 2024

halkeye commented Oct 15, 2024

gebn commented Oct 15, 2024

halkeye commented Oct 15, 2024

gebn commented Oct 15, 2024

halkeye commented Oct 17, 2024

		"github.com/gebn/unifibackup/v2/cmd/unifibackup/monitor"
		"github.com/gebn/unifibackup/v2/cmd/unifibackup/uploader"

	err := genmeta(CLI.Genmeta.Dir)
	if err != nil {
	if err := genmeta(CLI.Genmeta.Dir); err != nil {

	panic(ctx.Command())
	return fmt.Errorf("unknown command: %v", kongCtx.Command())

Switch to kong (aka support reading flags from env) #51

Are you sure you want to change the base?

Switch to kong (aka support reading flags from env) #51

Conversation

halkeye commented Oct 14, 2024

halkeye commented Oct 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halkeye commented Oct 14, 2024

gebn commented Oct 14, 2024

halkeye commented Oct 15, 2024

gebn commented Oct 15, 2024

halkeye commented Oct 15, 2024

gebn commented Oct 15, 2024

halkeye commented Oct 17, 2024