🌱 Replace disable-grouping with grouping option in clusterctl describe command #5550

DiptoChakrabarty · 2021-11-01T11:06:24Z

🌱

What this PR does / why we need it: This deprecates the disable-grouping(false) option in the clusterctl describe and uses the grouping(true) option as per the issue

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5346 (working on)

Comments in the files haven't been updated yet , wanted to know if I am taking the right approach for this

k8s-ci-robot · 2021-11-01T11:06:33Z

Hi @DiptoChakrabarty. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ykakarap · 2021-11-02T02:13:04Z

/rename 🌱 Replace disable-grouping with grouping option in clusterctl describe command

ykakarap · 2021-11-02T02:13:47Z

/cc

ykakarap · 2021-11-02T02:17:39Z

@DiptoChakrabarty Thanks for the PR!! 🚀

Two PR clean up items:

Please squash all the commits into a single one
Also, please rename the PR to have 🌱 at the beginning of the name. This way the workflow PRs will pass and then the actual test pipeline will be able to run.

ykakarap

@DiptoChakrabarty Can you also please update the tests to use the new Grouping field. The tests seem to still be using DisableGrouping.

ykakarap · 2021-11-02T02:20:16Z

cmd/clusterctl/cmd/describe_cluster.go

@@ -58,7 +58,7 @@ type describeClusterOptions struct {
 	showOtherConditions string
 	showMachineSets     bool
 	disableNoEcho       bool
-	disableGrouping     bool
+	Grouping            bool


Suggested change

Grouping bool

grouping bool

The field can remain private.

Done with this

ykakarap · 2021-11-02T02:20:34Z

cmd/clusterctl/cmd/describe_cluster.go

@@ -110,7 +110,7 @@ func init() {

 	describeClusterClusterCmd.Flags().BoolVar(&dc.disableNoEcho, "disable-no-echo", false, ""+
 		"Disable hiding of a MachineInfrastructure and BootstrapConfig when ready condition is true or it has the Status, Severity and Reason of the machine's object.")
-	describeClusterClusterCmd.Flags().BoolVar(&dc.disableGrouping, "disable-grouping", false,
+	describeClusterClusterCmd.Flags().BoolVar(&dc.Grouping, "grouping", true,


Suggested change

describeClusterClusterCmd.Flags().BoolVar(&dc.Grouping, "grouping", true,

describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "grouping", true,

ykakarap · 2021-11-02T02:20:43Z

cmd/clusterctl/cmd/describe_cluster.go

@@ -138,7 +138,7 @@ func runDescribeCluster(name string) error {
 		ShowOtherConditions: dc.showOtherConditions,
 		ShowMachineSets:     dc.showMachineSets,
 		DisableNoEcho:       dc.disableNoEcho,
-		DisableGrouping:     dc.disableGrouping,
+		Grouping:            dc.Grouping,


Suggested change

Grouping: dc.Grouping,

Grouping: dc.grouping,

cmd/clusterctl/client/describe.go

cmd/clusterctl/client/tree/discovery.go

ykakarap · 2021-11-02T02:24:52Z

cmd/clusterctl/client/tree/tree.go

 	// DisableGrouping disables grouping sibling objects in case the ready condition
 	// has the same Status, Severity and Reason


Same comment as the other places.

ykakarap · 2021-11-02T02:26:13Z

cmd/clusterctl/client/tree/tree.go

@@ -140,7 +140,7 @@ func (od ObjectTree) Add(parent, obj client.Object, opts ...AddObjectOption) (ad
 	// If it is requested that the child of this node should be grouped in case the ready condition
 	// has the same Status, Severity and Reason, add the GroupingObjectAnnotation to signal
 	// this to the presentation layer.
-	if addOpts.GroupingObject && !od.options.DisableGrouping {
+	if addOpts.GroupingObject && !od.options.Grouping {


Suggested change

if addOpts.GroupingObject && !od.options.Grouping {

if addOpts.GroupingObject && od.options.Grouping {

DiptoChakrabarty · 2021-11-02T04:32:39Z

Two PR clean up items:

Done both

cmd/clusterctl/client/tree/discovery_test.go

cmd/clusterctl/cmd/describe_cluster.go

cmd/clusterctl/client/describe.go

fabriziopandini · 2021-11-05T09:46:14Z

/ok-to-test

cmd/clusterctl/client/tree/discovery_test.go

fabriziopandini · 2021-11-05T09:59:44Z

cmd/clusterctl/cmd/describe_cluster.go

@@ -110,6 +111,9 @@ func init() {

 	describeClusterClusterCmd.Flags().BoolVar(&dc.disableNoEcho, "disable-no-echo", false, ""+
 		"Disable hiding of a MachineInfrastructure and BootstrapConfig when ready condition is true or it has the Status, Severity and Reason of the machine's object.")
+	describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "grouping", true,
+		"Disable grouping machines when ready condition has the same Status, Severity and Reason.")


Suggested change

"Disable grouping machines when ready condition has the same Status, Severity and Reason.")

"Groups machines when ready condition has the same Status, Severity and Reason.")

cmd/clusterctl/cmd/describe_cluster.go

fabriziopandini · 2021-11-05T11:07:06Z

Note.
What is important is to not remove the flag and deprecate it, given that a flag is a user contract.
Instead, it is fine to remove the field in DescribeClusterOptions.
@DiptoChakrabarty let me know if you need more clarifications about this point

DiptoChakrabarty · 2021-11-05T16:23:59Z

Instead, it is fine to remove the field in DescribeClusterOptions.

Okay so if I am understanding correctly instead of having the DisableGrouping option it is fine to use the Grouping option in the DescribeClusterOptions(

cluster-api/cmd/clusterctl/client/describe.go

Lines 26 to 50 in 0f347df

    
           type DescribeClusterOptions struct { 
        
           	// Kubeconfig defines the kubeconfig to use for accessing the management cluster. If empty, 
        
           	// default rules for kubeconfig discovery will be used. 
        
           	Kubeconfig Kubeconfig 
        
           	// Namespace where the workload cluster is located. If unspecified, the current namespace will be used. 
        
           	Namespace string 
        
           	// ClusterName to be used for the workload cluster. 
        
           	ClusterName string 
        
           	// ShowOtherConditions is a list of comma separated kind or kind/name for which we should add the ShowObjectConditionsAnnotation 
        
           	// to signal to the presentation layer to show all the conditions for the objects. 
        
           	ShowOtherConditions string 
        
           	// ShowMachineSets instructs the discovery process to include machine sets in the ObjectTree. 
        
           	ShowMachineSets bool 
        
           	// DisableNoEcho disable hiding MachineInfrastructure or BootstrapConfig objects if the object's ready condition is true 
        
           	// or it has the same Status, Severity and Reason of the parent's object ready condition (it is an echo) 
        
           	DisableNoEcho bool 
        
           	// DisableGrouping disable grouping machines objects in case the ready condition 
        
           	// has the same Status, Severity and Reason 
        
           	DisableGrouping bool

)

however in case of the disable-grouping flag that must be set to false as default opposite of the grouping flag set to true

So when disable-grouping option is provided grouping will not occur , however when grouping option is provided the output is grouped(all managed by the Grouping option in the DescribeClusterOptions) am I thinking correctly

fabriziopandini · 2021-11-05T16:59:00Z

yes, or in other words
DescribeClusterOptions.Grouping = (grouping flag) && !(disable-grouping flag)

DiptoChakrabarty · 2021-11-05T17:01:37Z

Okay working on it

fabriziopandini · 2021-11-08T11:23:08Z

cmd/clusterctl/cmd/describe_cluster.go

@@ -138,7 +140,7 @@ func runDescribeCluster(name string) error {
 		ShowOtherConditions: dc.showOtherConditions,
 		ShowMachineSets:     dc.showMachineSets,
 		DisableNoEcho:       dc.disableNoEcho,
-		DisableGrouping:     dc.disableGrouping,
+		Grouping:            dc.grouping,


this should be

Suggested change

Grouping: dc.grouping,

Grouping: dc.grouping && !dc.disableGrouping,

fabriziopandini · 2021-11-08T11:24:59Z

cmd/clusterctl/cmd/describe_cluster.go

-	describeClusterClusterCmd.Flags().BoolVar(&dc.disableGrouping, "disable-grouping", false,
+	describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "grouping", true,
+		"Groups machines when ready condition has the same Status, Severity and Reason.")
+	describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "disable-grouping", false,


You can't have two flags using the same variable

Suggested change

describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "disable-grouping", false,

describeClusterClusterCmd.Flags().BoolVar(&dc.disableGrouping, "disable-grouping", false,

Also the flag should be deprecated as per https://pkg.go.dev/github.com/spf13/pflag#readme-deprecating-a-flag-or-its-shorthand

yes marked it as deprecated

cmd/clusterctl/cmd/describe_cluster.go

ykakarap · 2021-11-08T18:37:59Z

@DiptoChakrabarty After addressing @fabriziopandini comments, please swash all the commits into a single commit. :)

DiptoChakrabarty · 2021-11-28T15:03:28Z

Hey I am not sure why the test is failing in the control plane grouping check case in this line

cluster-api/cmd/clusterctl/client/tree/discovery_test.go

Line 97 in 4b858f6

g.Expect(IsGroupingObject(obj)).To(BeTrue())

I tried checking the condition which enables the grouping annotation based on here

cluster-api/cmd/clusterctl/client/tree/tree.go

Lines 143 to 145 in 4b858f6

    
           if addOpts.GroupingObject && !od.options.DisableGrouping { 
        
           	addAnnotation(obj, GroupingObjectAnnotation, "True") 
        
           }

and what I can assume is that the second part of the condition is not being fulfilled in the control plane case (ie od.options.Grouping is not true in this case which has been modified in my PR)

I tried printing them while running it

this is coming as false , I am not sure what I am doing wrong here or what I need to fix

fabriziopandini · 2021-11-29T15:26:30Z

I will try to get a look at the error asap

fabriziopandini · 2021-11-30T10:29:45Z

@DiptoChakrabarty The "Discovery with default discovery settings" test is failing because it was written under the assumption that grouping should be disabled by default. With the changes introduced by this PR to achieve the default configuration in this change you now have to change

cluster-api/cmd/clusterctl/client/tree/discovery_test.go

Line 45 in 9cc0974

discoverOptions: DiscoverOptions{},

into

				discoverOptions: DiscoverOptions{
					Grouping: true,
				},

Also, please note that the flag deprecation is not yet properly implemented.
Could you change:

cluster-api/cmd/clusterctl/cmd/describe_cluster.go

Lines 115 to 116 in 4042172

    
           describeClusterClusterCmd.Flags().MarkDeprecated("disable-grouping", 
        
           	"--Deprecated use grouping option instead,Disable grouping machines when ready condition has the same Status, Severity and Reason.")

Into

	describeClusterClusterCmd.Flags().BoolVar(&dc.disableGrouping, "disable-grouping", false,
		"Disable grouping machines when ready condition has the same Status, Severity and Reason.")
	describeClusterClusterCmd.Flags().MarkDeprecated("disable-grouping",
		"use --grouping instead.")

If you don't mind, the last two nits:

cluster-api/cmd/clusterctl/cmd/describe_cluster.go

Line 86 in 4042172

clusterctl describe cluster test-1 --disable-grouping

should be changed into

		clusterctl describe cluster test-1 --grouping=false

And

cluster-api/cmd/clusterctl/cmd/describe_cluster.go

Line 90 in 4042172

clusterctl describe cluster test-1`),

Is missing the --disable-no-echo flag, so it should be modified into

		clusterctl describe cluster test-1 --disable-no-echo`),

With this changes everything should be ok (tested locally), so please squash commits

DiptoChakrabarty · 2021-11-30T10:41:45Z

grouping should be disabled by default. With the changes introduced by this PR to achieve the default configuration in this change you now have to change

okay sure doing it

DiptoChakrabarty · 2021-11-30T11:04:11Z

Updated them

fabriziopandini

@DiptoChakrabarty thanks!
@ykakarap PTAL

/lgtm

ykakarap

Overall LGTM.
Just some minor nits.

ykakarap · 2021-12-01T22:22:41Z

cmd/clusterctl/client/describe.go

-	// has the same Status, Severity and Reason
-	DisableGrouping bool
+	// Grouping groups machines objects in case the ready conditions
+	// have the same Status, Severity and Reason


nit

Suggested change

// have the same Status, Severity and Reason

// have the same Status, Severity and Reason.

ykakarap · 2021-12-01T22:26:29Z

cmd/clusterctl/client/tree/discovery.go

-	// has the same Status, Severity and Reason
-	DisableGrouping bool
+	// Grouping groups machine objects in case the ready conditions
+	// have the same Status, Severity and Reason


nit

Suggested change

// have the same Status, Severity and Reason

// have the same Status, Severity and Reason.

ykakarap · 2021-12-01T22:33:01Z

cmd/clusterctl/client/tree/tree_test.go

@@ -442,15 +442,15 @@ func Test_Add_setsGroupingObjectAnnotation(t *testing.T) {
 		{
 			name: "should add the annotation if requested to",


nit

Suggested change

name: "should add the annotation if requested to",

name: "should add the annotation if requested to and grouping is enabled",

… modify comments

DiptoChakrabarty · 2021-12-02T12:59:27Z

/retest

k8s-ci-robot · 2021-12-02T13:01:05Z

@DiptoChakrabarty: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`e333b97`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ykakarap

Thanks for all the fixes :)

/lgtm

ykakarap · 2021-12-02T15:58:25Z

@fabriziopandini Out of curiosity: When we want to replace public functions we deprecate the old function and add a new one. Do we also have to do the same for public fields in public types?

fabriziopandini · 2021-12-03T09:17:20Z

@fabriziopandini Out of curiosity: When we want to replace public functions we deprecate the old function and add a new one. Do we also have to do the same for public fields in public types?

We provide guarantees for API and CLI surface, while instead of for code we are allowed to do breaking changes between minor releases. more in https://github.com/kubernetes-sigs/cluster-api/blob/main/CONTRIBUTING.md

fabriziopandini · 2021-12-03T09:17:49Z

/lgtm
/approve

k8s-ci-robot · 2021-12-03T09:18:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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~~ [fabriziopandini]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 1, 2021

k8s-ci-robot requested review from JoelSpeed and sbueringer November 1, 2021 11:06

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 1, 2021

k8s-ci-robot requested a review from ykakarap November 2, 2021 02:13

ykakarap suggested changes Nov 2, 2021

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2021

DiptoChakrabarty force-pushed the describe branch from 58c991a to 3b7bf28 Compare November 2, 2021 04:28

DiptoChakrabarty changed the title ~~Replace disable-grouping with grouping option in clusterctl describe command 🌱~~ 🌱 Replace disable-grouping with grouping option in clusterctl describe command Nov 2, 2021

fabriziopandini reviewed Nov 2, 2021

View reviewed changes

cmd/clusterctl/client/tree/discovery_test.go Outdated Show resolved Hide resolved

cmd/clusterctl/cmd/describe_cluster.go Show resolved Hide resolved

cmd/clusterctl/cmd/describe_cluster.go Show resolved Hide resolved

cmd/clusterctl/client/describe.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 5, 2021

fabriziopandini reviewed Nov 5, 2021

View reviewed changes

fabriziopandini reviewed Nov 8, 2021

View reviewed changes

DiptoChakrabarty force-pushed the describe branch from 4042172 to 303d77d Compare November 30, 2021 10:54

fabriziopandini reviewed Nov 30, 2021

View reviewed changes

k8s-ci-robot assigned fabriziopandini Nov 30, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2021

fabriziopandini mentioned this pull request Dec 1, 2021

Possible improvements for clusterctl describe #5346

Closed

ykakarap reviewed Dec 1, 2021

View reviewed changes

replace disable-grouping option with grouping option update tests and…

e333b97

… modify comments

DiptoChakrabarty force-pushed the describe branch from 303d77d to e333b97 Compare December 2, 2021 12:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2021

ykakarap approved these changes Dec 2, 2021

View reviewed changes

k8s-ci-robot assigned ykakarap Dec 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021

k8s-ci-robot merged commit 6780524 into kubernetes-sigs:main Dec 3, 2021

k8s-ci-robot added this to the v1.1 milestone Dec 3, 2021

DiptoChakrabarty deleted the describe branch December 29, 2021 14:52

DiptoChakrabarty mentioned this pull request Mar 23, 2022

REQUEST: New membership for DiptoChakrabarty kubernetes/org#3335

Closed

9 tasks

	describeClusterClusterCmd.Flags().BoolVar(&dc.Grouping, "grouping", true,
	describeClusterClusterCmd.Flags().BoolVar(&dc.grouping, "grouping", true,

		// DisableGrouping disables grouping sibling objects in case the ready condition
		// has the same Status, Severity and Reason

	if addOpts.GroupingObject && !od.options.Grouping {
	if addOpts.GroupingObject && od.options.Grouping {

	"Disable grouping machines when ready condition has the same Status, Severity and Reason.")
	"Groups machines when ready condition has the same Status, Severity and Reason.")

	Grouping: dc.grouping,
	Grouping: dc.grouping && !dc.disableGrouping,

	// have the same Status, Severity and Reason
	// have the same Status, Severity and Reason.

		@@ -442,15 +442,15 @@ func Test_Add_setsGroupingObjectAnnotation(t *testing.T) {
		{
		name: "should add the annotation if requested to",

🌱 Replace disable-grouping with grouping option in clusterctl describe command #5550

🌱 Replace disable-grouping with grouping option in clusterctl describe command #5550

Conversation

DiptoChakrabarty commented Nov 1, 2021

k8s-ci-robot commented Nov 1, 2021

ykakarap commented Nov 2, 2021

ykakarap commented Nov 2, 2021

ykakarap commented Nov 2, 2021

ykakarap left a 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

DiptoChakrabarty commented Nov 2, 2021

fabriziopandini commented Nov 5, 2021

Choose a reason for hiding this comment

fabriziopandini commented Nov 5, 2021

DiptoChakrabarty commented Nov 5, 2021

fabriziopandini commented Nov 5, 2021

DiptoChakrabarty commented Nov 5, 2021

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

ykakarap commented Nov 8, 2021

DiptoChakrabarty commented Nov 28, 2021

fabriziopandini commented Nov 29, 2021

fabriziopandini commented Nov 30, 2021

DiptoChakrabarty commented Nov 30, 2021

DiptoChakrabarty commented Nov 30, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

ykakarap left a 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

DiptoChakrabarty commented Dec 2, 2021

k8s-ci-robot commented Dec 2, 2021 • edited

ykakarap left a comment

Choose a reason for hiding this comment

ykakarap commented Dec 2, 2021

fabriziopandini commented Dec 3, 2021

fabriziopandini commented Dec 3, 2021

k8s-ci-robot commented Dec 3, 2021

k8s-ci-robot commented Dec 2, 2021 •

edited