Add DryCLI status comparison #329

yiraeChristineKim · 2025-02-04T15:06:14Z

Ref: https://issues.redhat.com/browse/ACM-17517

yiraeChristineKim · 2025-02-04T16:49:27Z

pkg/dryrun/dryrun_utils.go

+	isStatusMatch := compareStatusObj(cmd, true, "", inputStatus, resultStatus)
+
+	if isStatusMatch {
+		cmd.Println(successColor("\033[1m Expected status matches the actual status \033[0m"))


If we'll have terminal escapes for bold, I wonder if we should just "manually" do the terminal escapes for the colors ourselves, as opposed to using that library. How much trouble do you think that would it be?

Let me try I don't think it is big deal

Fixed Could you check

I think that's looking good. What do you think about a --no-colors option that would turn off those escapes?

yiraeChristineKim · 2025-02-04T18:04:00Z

/cc @JustinKuli

JustinKuli

Sorry for the many small comments. Overall this looks really great.

I personally like the colors, but I feel like it's common to give a --no-color option. Especially if this is a tool used in someone's CI, it can be a headache to look at the output with a bunch of terminal escape codes.

I think the exit code of the CLI is still just whether the result is compliant or not... I think we'd want to adjust that so that if the user provided a desired status, the exit code is zero if the status matches.

go.mod

pkg/dryrun/cmd.go

JustinKuli · 2025-02-04T19:22:11Z

pkg/dryrun/cmd.go


+	cmd.Flags().StringVarP(
+		&d.configStatus,
+		"config-status",


What do you think of "desired-status"? Would that be more clear?

I just worry that "config-status" makes it sound like it configures some piece of the status somehow.

JustinKuli · 2025-02-04T21:00:01Z

pkg/dryrun/dryrun_utils.go

+	isStatusMatch := compareStatusObj(cmd, true, "", inputStatus, resultStatus)
+
+	if isStatusMatch {
+		cmd.Println(successColor("\033[1m Expected status matches the actual status \033[0m"))


If we'll have terminal escapes for bold, I wonder if we should just "manually" do the terminal escapes for the colors ourselves, as opposed to using that library. How much trouble do you think that would it be?

JustinKuli · 2025-02-04T21:02:58Z

pkg/dryrun/dryrun_utils.go

+			}
+
+			switch typedV[0].(type) {
+			case map[interface{}]interface{}, map[string]interface{}:


Isn't map[string]interface{} a sub-case of map[interface{}]interface{}? Is it necessary to make it a specific case?

It seems like other pieces here want to only deal with map[string]interface. So I think that could be made a case on its own, and then if the next case was map[interface{}]interface{}, that would catch the errors all together. Might be cleaner (assuming it actually works like that).

Sorry, I don’t understand why these are separated—the code would be duplicated. Could you suggest an alternative implementation?

pkg/dryrun/dryrun_utils.go

JustinKuli · 2025-02-04T21:30:33Z

pkg/dryrun/dryrun_utils.go

+
+					mapObj, ok := convertMapStringKey(obj).(map[string]interface{})
+					if !ok {
+						failedParsePrint(cmd, path)


I think we can consider it not a match if we fail to parse something. Warnings are too likely to be ignored in my opinion.

pkg/dryrun/dryrun_utils.go

JustinKuli · 2025-02-04T21:36:34Z

pkg/dryrun/dryrun_utils.go

+}
+
+func successMatchArrayPrint(cmd *cobra.Command, path string) {
+	cmd.Println(successColor(fmt.Sprintf("%s is founded", path)))


Suggested change

cmd.Println(successColor(fmt.Sprintf("%s is founded", path)))

cmd.Println(successColor(fmt.Sprintf("%s matches", path)))

I'm worried this message is very repetitive. Maybe it shouldn't be printed at all?

should we remove

if !allMatch { errorMatchArrayPrint(cmd, path) isStatusMatch = false }

This case print too?

pkg/dryrun/testdata/test_config_status/output.txt

pkg/dryrun/dryrun_utils.go

JustinKuli · 2025-02-05T19:01:49Z

pkg/dryrun/dryrun_utils.go

+	isStatusMatch := compareStatusObj(cmd, true, "", inputStatus, resultStatus)
+
+	if isStatusMatch {
+		cmd.Println(successColor("\033[1m Expected status matches the actual status \033[0m"))


I think that's looking good. What do you think about a --no-colors option that would turn off those escapes?

pkg/dryrun/dryrun_utils.go

Ref: https://issues.redhat.com/browse/ACM-17517 Signed-off-by: yiraeChristineKim <[email protected]>

JustinKuli

LGTM! Thanks

openshift-ci · 2025-02-10T15:11:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, yiraeChristineKim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JustinKuli,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes approved labels Feb 4, 2025

yiraeChristineKim force-pushed the ACM-17517 branch 5 times, most recently from 331c86f to b9fe98d Compare February 4, 2025 16:48

yiraeChristineKim commented Feb 4, 2025

View reviewed changes

yiraeChristineKim force-pushed the ACM-17517 branch from b9fe98d to 3cfd7ad Compare February 4, 2025 17:16

yiraeChristineKim marked this pull request as ready for review February 4, 2025 18:03

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 4, 2025

openshift-ci bot requested review from dhaiducek and gparvin February 4, 2025 18:03

openshift-ci bot requested a review from JustinKuli February 4, 2025 18:04

yiraeChristineKim force-pushed the ACM-17517 branch from 3cfd7ad to 2d3a7e6 Compare February 4, 2025 19:07

JustinKuli requested changes Feb 4, 2025

View reviewed changes

openshift-ci bot assigned JustinKuli Feb 4, 2025

yiraeChristineKim force-pushed the ACM-17517 branch 3 times, most recently from da61bb8 to 6c23b46 Compare February 5, 2025 16:36

yiraeChristineKim requested a review from JustinKuli February 5, 2025 17:09

JustinKuli reviewed Feb 5, 2025

View reviewed changes

yiraeChristineKim force-pushed the ACM-17517 branch 2 times, most recently from dbc8f98 to e25fa04 Compare February 6, 2025 15:55

yiraeChristineKim requested a review from JustinKuli February 6, 2025 16:05

Add DryCLI status comparison

69f9424

Ref: https://issues.redhat.com/browse/ACM-17517 Signed-off-by: yiraeChristineKim <[email protected]>

yiraeChristineKim force-pushed the ACM-17517 branch from e25fa04 to 69f9424 Compare February 6, 2025 16:05

JustinKuli approved these changes Feb 10, 2025

View reviewed changes

openshift-ci bot added the lgtm label Feb 10, 2025

openshift-merge-bot bot merged commit bf7ec7d into open-cluster-management-io:main Feb 10, 2025
13 checks passed

magic-mirror-bot bot mentioned this pull request Feb 10, 2025

🤖 Sync from open-cluster-management-io/config-policy-controller: #329 stolostron/config-policy-controller#1184

Merged

	cmd.Println(successColor(fmt.Sprintf("%s is founded", path)))
	cmd.Println(successColor(fmt.Sprintf("%s matches", path)))

Add DryCLI status comparison #329

Add DryCLI status comparison #329

Uh oh!

Conversation

yiraeChristineKim commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim commented Feb 4, 2025

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

yiraeChristineKim commented Feb 4, 2025 •

edited

Loading

yiraeChristineKim Feb 5, 2025 •

edited

Loading