feat: Add CLI option --format to disconnect command #108

jirihnidek · 2024-05-02T18:04:22Z

It is possible to print output of disconnect command in machine-readable format. The --format requires argument and it could be only "json" ATM. It follows practice of status command.
Refactored code a little to share some code with status command.

subpop · 2024-05-02T18:51:18Z

I like the use of the Before function handler. That's a good idea to validate flags and user input!

jirihnidek · 2024-05-03T07:27:07Z

BTW: I would like to add --format CLI option to connect command too, but I would like to do some refactoring of main.go first. This file is just too big and I would like to split it into connect.go and disconnect.go. Each command should be in separate file.

subpop · 2024-05-06T16:47:10Z

disconnect, connect, status all could take advantage of a --format flag. This duplication leads me to believe there should be a global/top-level --format flag that all subcommands can use. From within the Action of a subcommand, flags defined on the parent are accessed from within the cli.Context using their name like any other flag. For example, consider the following program:

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli/v2"
)

func main() {
	app := cli.NewApp()
	app.Name = "rhc"

	app.Flags = []cli.Flag{
		&cli.StringFlag{
			Name: "format",
		},
	}
	app.Commands = []*cli.Command{
		{
			Name: "connect",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name: "username",
				},
			},
			Action: func(ctx *cli.Context) error {
				fmt.Printf("username: %v\n", ctx.String("username"))
				fmt.Printf("format: %v\n", ctx.String("format"))
				return nil
			},
		},
	}

	if err := app.Run(os.Args); err != nil {
		log.Fatal(err)
	}
}

When invoked like so: rhc --format hello connect, it outputs:

format: hello

However, when invoked like so: rhc connect --format hello, it will error. The flags defined in a parent command must immediately follow the command on which they are defined.

Have you considered adding --format as a top-level flag?

jirihnidek · 2024-05-07T07:02:45Z

I agree that it would be good to have --format json as top-level flag, but I would like to have support for machine-readable output for commands. Should I implement --format json for connect command as part of this PR?

What bothers me is a fact that we will break compatibility with rhc status --format json.

TBH: I don't like the behavior of github.com/urfave/cli that you have to use flags of parent command immediately after main command. This is something unusual.

There is alfa version (v3) of github.com/urfave/cli (we still use v2) ... I will try it if the behavior is still the same in v3.

jirihnidek · 2024-05-07T09:58:17Z

The v3 support persistent flags introduces in this PR: urfave/cli#1568

The code looks like this:

package main

import (
	"context"
	"fmt"
	"github.com/urfave/cli/v3"
	"log"
	"os"
)

func main() {
	cmd := cli.Command{
		Name: "rhc",
		Commands: []*cli.Command{
			{
				Name: "connect",
				Flags: []cli.Flag{
					&cli.StringFlag{
						Name: "username",
					},
				},
				Action: func(ctx context.Context, command *cli.Command) error {
					fmt.Printf("username: %s\n", command.String("username"))
					fmt.Printf("format: %s\n", command.String("format"))
					return nil
				},
			},
		},
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name:       "format",
				Persistent: true,
			},
		},
	}

	err := cmd.Run(context.Background(), os.Args)
	if err != nil {
		log.Fatal(err)
	}
}

If a flag is marked as persistent, then you can use global flag after sub-command like this:

jhnidek@thinkpad-p1:~/tmp/go_cli$ ./main --format json connect --username pepa
username: pepa
format: json
jhnidek@thinkpad-p1:~/tmp/go_cli$ ./main connect --username pepa --format json
username: pepa
format: json
jhnidek@thinkpad-p1:~/tmp/go_cli$ ./main --format xml connect --username pepa --format json
username: pepa
format: json

Unfortunately this feature hasn't been back-ported to v2.

subpop · 2024-05-08T15:31:58Z

Oh, that's exactly what we should use. Let's proceed with separate --format flags for each subcommand, and when we update to cli/v3, we can refactor the code to use a single, persistent flag. That way we can support --format for the subcommands that need them and maintain compatibility with our CLI when we do eventually refactor.

main.go

subpop · 2024-05-08T16:22:56Z

main.go

+// disconnectAction tries to stop rhscd service, disconnect from Red Hat Insights, and finally
 // it unregister system from Red Hat Subscription Management


Suggested change

// disconnectAction tries to stop rhscd service, disconnect from Red Hat Insights, and finally

// it unregister system from Red Hat Subscription Management

// disconnectAction tries to stop rhcd service, disconnect from Red Hat Insights, and

// finally it unregisters system from Red Hat Subscription Management

main.go

subpop

I noticed a significant amount of conditional checks on the value of uiSettings.isMachineReadable, and some data structure transformation between DisconnectResult and error. I recommend looking into a way to make DisconnectResult conform to the error interface. I think that could result in reducing a lot of the conditional checks, and might streamline some of the code paths that are trying to transform a DisconnectResult into and error.

You might even consider making a generic Result interface type that DisconnectResult can implement. This could be expanded to other command results such as status (with a StatusResult) and connect (with a ConnectResult.

jirihnidek · 2024-05-09T15:30:10Z

I agree that there is too many checks of uiSettings.isMachineReadable. I would like to simplify this. Problem is that we try to print some progress (spinner, progress messaged, colors, etc.), when --format CLI is not used. For this reason we just cannot defer printing results in all cases.

The DisconnectResult can also contain many error messages from stopping yggdrasil service, insights-client, rhsm.service. Thus, I still do not understand how implementing func (err DisconnectResult) Error() interface could help to simplify the code. The DisconnectResult is never converted to error.

main.go

jirihnidek · 2024-05-13T09:19:37Z

Refactored code a little bit. I hope that it is better now.

jirihnidek · 2024-08-14T08:47:32Z

@subpop Hi, can you please review this PR again?

subpop

Looks good over all. I just have a couple very minor questions.

subpop · 2024-08-20T15:27:51Z

main.go

+	if uiSettings.isMachineReadable {
+		defer func(disconnectResult *DisconnectResult) {
+			// When exit code is zero, then print machine-readable output
+			// When exit code has non-zero value, then disconnectResult is returned as a error


This comment doesn't see to align with the behavior of the code. Is it left over
from some previous code? This is one of the pitfalls of using comments to
describe the algorithm of the code.

subpop · 2024-08-20T15:29:12Z

main.go

+				// change returned error to CLI exit error to be able to set exit code to
+				// a non-zero value
+				if err != nil {
+					panic(fmt.Errorf("unable to print status as %s document: %s", format, err.Error()))


Typically, it's not necessary to explicitly call Error() on an error type. This
should be sufficient.

Suggested change

panic(fmt.Errorf("unable to print status as %s document: %s", format, err.Error()))

panic(fmt.Errorf("unable to print status as %v document: %v", format, err))

subpop · 2024-08-20T15:30:00Z

main.go

+		}(&disconnectResult)
+	}
+
+	disconnectResult.exitCode = 0


Is this necessary? The zero value of an int is 0 already.

Refactored code to not duplicate code. Only Error() interface is used now.

Nice refactor! I like the change and use of Error interface.

subpop · 2024-08-26T14:19:09Z

There appear to be some valid lint errors related to printf format strings.

subpop · 2024-08-26T16:22:24Z

main.go

+
+// rhcPrintf is method for printing human-readable output. It suppresses output, when
+// machine-readable format is used.
+func rhcPrintf(format string, a ...interface{}) {


The name of this function doesn't indicate its purpose clearly to me. Could we
call it something like interactivePrintf?

* It is possible to print output of disconnect command in machine-readable format. The --format requires argument and it could be only "json" ATM. It follows practice of status command. * Refactored code a little to share some code with status command.

* When --format json is used, then use only Error() interface for printing disconnect result

* Fix some code style issues and make code linter more happy

subpop

OK. This looks good to me!

subpop self-requested a review May 8, 2024 16:27

subpop requested changes May 8, 2024

View reviewed changes

subpop reviewed May 8, 2024

View reviewed changes

jirihnidek commented May 9, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

jirihnidek commented May 9, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

subpop reviewed May 10, 2024

View reviewed changes

main.go Show resolved Hide resolved

jirihnidek force-pushed the jhnidek/disconnect_json_output branch from 2fe764b to bda0cad Compare May 13, 2024 09:17

jirihnidek force-pushed the jhnidek/disconnect_json_output branch from bda0cad to 0f31209 Compare May 14, 2024 08:05

jirihnidek requested a review from subpop June 13, 2024 14:47

jirihnidek mentioned this pull request Jul 23, 2024

feat: Add --format json for connect command #127

Merged

subpop requested changes Aug 20, 2024

View reviewed changes

jirihnidek force-pushed the jhnidek/disconnect_json_output branch from 0f31209 to ebea78b Compare August 21, 2024 13:22

subpop requested changes Aug 26, 2024

View reviewed changes

jirihnidek added 2 commits September 2, 2024 10:13

refactor: Refactoring of disconnect using --format json

1490c11

* When --format json is used, then use only Error() interface for printing disconnect result

jirihnidek force-pushed the jhnidek/disconnect_json_output branch from ebea78b to d79ca95 Compare September 2, 2024 09:01

fix: Do not use non-constant format string with Printf/Errorf

1b53bd3

* Fix some code style issues and make code linter more happy

jirihnidek force-pushed the jhnidek/disconnect_json_output branch from d79ca95 to 1b53bd3 Compare September 3, 2024 12:03

subpop approved these changes Sep 4, 2024

View reviewed changes

subpop merged commit 87c37da into main Sep 4, 2024
23 of 24 checks passed

subpop deleted the jhnidek/disconnect_json_output branch September 4, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add CLI option --format to disconnect command #108

feat: Add CLI option --format to disconnect command #108

jirihnidek commented May 2, 2024

subpop commented May 2, 2024

jirihnidek commented May 3, 2024

subpop commented May 6, 2024

jirihnidek commented May 7, 2024

jirihnidek commented May 7, 2024 •

edited

Loading

subpop commented May 8, 2024

subpop May 8, 2024

subpop left a comment

jirihnidek commented May 9, 2024 •

edited

Loading

jirihnidek commented May 13, 2024 •

edited

Loading

jirihnidek commented Aug 14, 2024

subpop left a comment

subpop Aug 20, 2024

subpop Aug 20, 2024

subpop Aug 20, 2024

jirihnidek Aug 21, 2024

subpop Aug 26, 2024

subpop commented Aug 26, 2024

subpop Aug 26, 2024

subpop left a comment

		// disconnectAction tries to stop rhscd service, disconnect from Red Hat Insights, and finally
		// it unregister system from Red Hat Subscription Management

	panic(fmt.Errorf("unable to print status as %s document: %s", format, err.Error()))
	panic(fmt.Errorf("unable to print status as %v document: %v", format, err))

feat: Add CLI option --format to disconnect command #108

feat: Add CLI option --format to disconnect command #108

Conversation

jirihnidek commented May 2, 2024

subpop commented May 2, 2024

jirihnidek commented May 3, 2024

subpop commented May 6, 2024

jirihnidek commented May 7, 2024

jirihnidek commented May 7, 2024 • edited Loading

subpop commented May 8, 2024

subpop May 8, 2024

Choose a reason for hiding this comment

subpop left a comment

Choose a reason for hiding this comment

jirihnidek commented May 9, 2024 • edited Loading

jirihnidek commented May 13, 2024 • edited Loading

jirihnidek commented Aug 14, 2024

subpop left a comment

Choose a reason for hiding this comment

subpop Aug 20, 2024

Choose a reason for hiding this comment

subpop Aug 20, 2024

Choose a reason for hiding this comment

subpop Aug 20, 2024

Choose a reason for hiding this comment

jirihnidek Aug 21, 2024

Choose a reason for hiding this comment

subpop Aug 26, 2024

Choose a reason for hiding this comment

subpop commented Aug 26, 2024

subpop Aug 26, 2024

Choose a reason for hiding this comment

subpop left a comment

Choose a reason for hiding this comment

jirihnidek commented May 7, 2024 •

edited

Loading

jirihnidek commented May 9, 2024 •

edited

Loading

jirihnidek commented May 13, 2024 •

edited

Loading