Add `pgo show ha` command and add combined output to `pgo show` #77

tjmoore4 · 2023-10-30T21:45:33Z

Adds a new subcommand to pgo show, ha, which provides the patronictl list command info when invoked.
Update remaining KUTTL tests to Postgres 15
Adds the default output from pgo show backup and pgo show ha to the pgo show command.
Adds enum based output format flag validation for pgo show backup --output

Issue: PGO-13

docs/content/reference/pgo_show_ha.md

internal/cmd/show.go

docs/content/reference/pgo_show.md

docs/content/reference/pgo_show_ha.md

internal/cmd/show.go

benjaminjb · 2023-10-31T13:27:42Z

internal/cmd/show.go

+	cmdShow.Args = cobra.ExactArgs(1)
+
+	// Define the 'show backup' command
+	cmdShow.RunE = func(cmd *cobra.Command, args []string) error {


This is pretty tidy (helped by the choice of spinning off getPrimaryExec as a helper func), but I wonder if it would be better to do something like:

<print backup header> newShowBackupCommand(config).Execute() <print HA header> newShowHACommand(config).Execute()

That way this show will never drift from those child func implementations. (And if we need to set args before executing, there's SetArgs -- though I wonder if a child cmd's execute inherits those args already?)

👍 Good idea. I'll give that a shot.

That seems to work well, with a couple of adjustments:

using Execute doesn't work because the sub-command will treat both show and the cluster name as arguments. The way to do this is to invoke RunE directly.

I had to pass through cmd and args. This is fine as both sub-commands take one and only one argument, but this could get trickier in the future if different commands required different arguments. In that case, I think we would need to use SetArgs as you suggested.

Hmmm, just to check, if you do

showCmd := newShowHACommand(config) err := showCmd.Execute()

it fails because it's only expecting one argument?

Oh, I wonder if this is the issue here: https://github.com/spf13/cobra/blob/v1.7.0/command.go#L1011C1-L1012

From testing, what I was seeing is that the show command had my cluster name as its only arg, but once show backup was invoked, the arg list contained show and the cluster name. Since the command can only accept one argument, it would fail immediately.

I mean, we might be wanting to "run show ha" [logic/behavior] without invoking Cobra to do it.

Ok, gotcha. That's how I had it previously (which is what started this comment thread). The benefit with this approach is that we only define the logic once, but it wouldn't be hard to revert to the older approach if that's what we'd like to do here. What is the downside to invoking Cobra in this way?

I think the idea that Chris is pointing at is:

ORIGINAL

showBackup: code to do A showHA: code to do B show: code to do A and code to do B

MY IDEA

showBackup: code to do A showHA: code to do B show: showBackup(), showHA() # invoke cobra

CHRIS'S IDEA

showBackup: backupFunc() showHA: haFunc() show: backupFunc(), haFunc() # just call the logic

This is like the idea that I'm trying out in the separate-concerns branch here.

The benefit of invoking Cobra is that it does all the pre and post, etc., if we need it, which we currently don't. (Separating out the logic from the Cobra does make it easier to test without calling KUTTL.) But this does keep us from repeating logic.

Ok, we might need to touch base to make sure I'm following. It looks like we'd need to pull in a lot from the cobra implementation in a way that seems cluttered to me, but maybe I'm missing something.

Pushed updates based on discussion with @benjaminjb.

Adds a new subcommand to pgo show, 'ha', which provides the 'patronictl list' command info when invoked. Issue: PGO-13

internal/cmd/exec.go

docs/content/reference/pgo_show_ha.md

This commit adds the default output from 'pgo show backup' and 'pgo show ha' to the 'pgo show' command. Issue: PGO-13

Adds enum based output format flag validation as well as a KUTTL TestStep to check for the desired behavior.

internal/util/enum.go

benjaminjb · 2023-11-01T19:48:59Z

internal/cmd/show.go

+		} else {
+			cmd.Printf(stdout)
+			if stderr != "" {
+				cmd.Printf("\nError returned: %s\n", stderr)


Suggested change

cmd.Printf("\nError returned: %s\n", stderr)

cmd.Printf("\nError returned from backup: %s\n", stderr)

?

Or will the stderr be clear?

It should generally be clear, but I don't think that's guaranteed. Maybe Error returned from pgBackRest would be clearer?

benjaminjb · 2023-11-01T19:54:46Z

internal/util/enum.go

+
+// String is used both by fmt.Print and by Cobra in help text
+func (e *patroniFormat) String() string {
+	return string(*e)
+}
+
+// Set must have pointer receiver so it doesn't change the value of a copy


I wonder if this file could use some code comments up top to explain where the form for this type is coming from. (Is it here: https://github.com/spf13/pflag/blob/master/flag.go#L187 ?)

Sure, I can add a comment. That's the right part, I think linking to https://pkg.go.dev/github.com/spf13/pflag#Value might work.

internal/cmd/exec.go

internal/cmd/show.go

benjaminjb

No blockers

Issue: PGO-13

tjmoore4 requested review from benjaminjb and tony-landreth October 30, 2023 21:45

tjmoore4 force-pushed the show-patroni branch from 21406e1 to 75e9a01 Compare October 30, 2023 21:48

tony-landreth reviewed Oct 31, 2023

View reviewed changes

docs/content/reference/pgo_show_ha.md Outdated Show resolved Hide resolved

tony-landreth reviewed Oct 31, 2023

View reviewed changes

docs/content/reference/pgo_show_ha.md Outdated Show resolved Hide resolved

tony-landreth reviewed Oct 31, 2023

View reviewed changes

internal/cmd/show.go Outdated Show resolved Hide resolved

tjmoore4 force-pushed the show-patroni branch 2 times, most recently from 5cc312b to e0b344d Compare October 31, 2023 15:49

benjaminjb reviewed Oct 31, 2023

View reviewed changes

tjmoore4 force-pushed the show-patroni branch from e0b344d to e0b8162 Compare October 31, 2023 16:50

tjmoore4 added 2 commits October 31, 2023 15:01

Add 'pgo show ha' command

87ea6d9

Adds a new subcommand to pgo show, 'ha', which provides the 'patronictl list' command info when invoked. Issue: PGO-13

Update remaining KUTTL tests to Postgres 15

c7c06a1

tjmoore4 force-pushed the show-patroni branch from e0b8162 to da8e23a Compare October 31, 2023 19:01

benjaminjb reviewed Oct 31, 2023

View reviewed changes

internal/cmd/exec.go Outdated Show resolved Hide resolved

internal/cmd/exec.go Outdated Show resolved Hide resolved

cbandy reviewed Oct 31, 2023

View reviewed changes

docs/content/reference/pgo_show_ha.md Outdated Show resolved Hide resolved

tjmoore4 force-pushed the show-patroni branch from da8e23a to 80f2230 Compare October 31, 2023 22:47

tjmoore4 added 2 commits October 31, 2023 18:52

Add all sub-command output to 'pgo show' command

101e8fd

This commit adds the default output from 'pgo show backup' and 'pgo show ha' to the 'pgo show' command. Issue: PGO-13

Add output flag validation for show backup command

51c894b

Adds enum based output format flag validation as well as a KUTTL TestStep to check for the desired behavior.

tjmoore4 force-pushed the show-patroni branch from 80f2230 to 51c894b Compare October 31, 2023 22:52

benjaminjb reviewed Nov 1, 2023

View reviewed changes

internal/util/enum.go Outdated Show resolved Hide resolved

internal/util/enum.go Outdated Show resolved Hide resolved

tjmoore4 force-pushed the show-patroni branch from c903c97 to d13dd37 Compare November 1, 2023 16:57

benjaminjb reviewed Nov 1, 2023

View reviewed changes

internal/cmd/exec.go Show resolved Hide resolved

benjaminjb reviewed Nov 1, 2023

View reviewed changes

internal/cmd/show.go Outdated Show resolved Hide resolved

benjaminjb reviewed Nov 1, 2023

View reviewed changes

internal/cmd/show.go Outdated Show resolved Hide resolved

benjaminjb approved these changes Nov 1, 2023

View reviewed changes

Refactor 'pgo show' functions and update help output

20fd95a

Issue: PGO-13

tjmoore4 force-pushed the show-patroni branch from d13dd37 to 20fd95a Compare November 1, 2023 20:47

tjmoore4 merged commit 80900a2 into CrunchyData:main Nov 1, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `pgo show ha` command and add combined output to `pgo show` #77

Add `pgo show ha` command and add combined output to `pgo show` #77

tjmoore4 commented Oct 30, 2023 •

edited

Loading

benjaminjb Oct 31, 2023

tjmoore4 Oct 31, 2023

tjmoore4 Oct 31, 2023

benjaminjb Oct 31, 2023

tjmoore4 Oct 31, 2023

cbandy Nov 1, 2023

tjmoore4 Nov 1, 2023

benjaminjb Nov 1, 2023

tjmoore4 Nov 1, 2023

tjmoore4 Nov 1, 2023

benjaminjb Nov 1, 2023

tjmoore4 Nov 1, 2023

benjaminjb Nov 1, 2023

tjmoore4 Nov 1, 2023 •

edited

Loading

benjaminjb left a comment

	cmd.Printf("\nError returned: %s\n", stderr)
	cmd.Printf("\nError returned from backup: %s\n", stderr)

Add pgo show ha command and add combined output to pgo show #77

Add pgo show ha command and add combined output to pgo show #77

Conversation

tjmoore4 commented Oct 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

tjmoore4 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

benjaminjb left a comment

Choose a reason for hiding this comment

Add `pgo show ha` command and add combined output to `pgo show` #77

Add `pgo show ha` command and add combined output to `pgo show` #77

tjmoore4 commented Oct 30, 2023 •

edited

Loading

tjmoore4 Nov 1, 2023 •

edited

Loading