Refactor pkg/subctl/cmd/gather -> internal/gather #2007

Jaanki · 2022-04-07T12:00:20Z

This PR does the following:

Adds config to cluster.Info
Uses execute.OnMultiCluster functionality
Uses constants defined in internal/constants.go

Related to #1329

Signed-off-by: Janki Chhatbar [email protected]

submariner-bot · 2022-04-07T12:00:22Z

🤖 Created branch: z_pr2007/Jaanki/refactor_gather
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

tpantelis

So it looks like you copied code to internal/gather but it's not being used.

Jaanki · 2022-04-08T07:03:12Z

So it looks like you copied code to internal/gather but it's not being used.

Yes. e2e is failing for some reason so didn't push more commits.

pkg/cluster/info.go

tpantelis

To maintain git history, we should first move the files to internal/gather.

dfarrell07 · 2022-04-26T18:39:17Z

To maintain git history, we should first move the files to internal/gather.

We have a pointer for this guidance at https://submariner.io/development/code-review/#rename-and-edit-in-separate-commits FYI. I think it was created because of very similar feedback on similar refactoring a few months ago.

Jaanki · 2022-04-27T10:08:39Z

To maintain git history, we should first move the files to internal/gather.

git mv moves the files from pkg/subctl/cmd/gather/. to internal/gather. I want the files to be copied and not moved.

tpantelis · 2022-04-27T11:10:23Z

git mv moves the files from pkg/subctl/cmd/gather/. to internal/gather. I want the files to be copied and not moved.

Why? Copying loses the git history.

git log -- internal/gather/cni.go

commit d11b28efd3454e88ec8a69826270366bb4f24133
Author: Janki Chhatbar <[email protected]>
Date:   Fri Apr 22 11:45:28 2022 +0530

    Copy pkg/subctl/cmd/gather -> internal/gather
    
    Signed-off-by: Janki Chhatbar <[email protected]>

The deceiving thing tho is that the diff in the GtHub UI shows that it was renamed w/o changes.

skitt · 2022-05-03T12:34:20Z

internal/gather/gather.go

+	ModuleFlags          map[string]bool
+	TypeFlags            map[string]bool


These should be string slices, not maps; that would make it easier to use them as Cobra flags directly (and we don’t care about the bool).

Commit 0e5e8a4.

skitt · 2022-05-03T12:36:09Z

internal/gather/gather.go

+var ModuleFlags = map[string]bool{
+	component.Connectivity:     false,
+	component.ServiceDiscovery: false,
+	component.Broker:           false,
+	component.Operator:         false,
+}


This shouldn’t be necessary, the same information is effectively present in gatherFuncs; perhaps a GetSupportedModules() []string function could be used instead, or an IsModuleSupported(module string) bool function.

Made it a set with 0e5e8a4.

skitt · 2022-05-03T12:40:07Z

internal/gather/gather.go

+var TypeFlags = map[string]bool{
+	"logs":      false,
+	"resources": false,
+}


Likewise, this should be hidden behind a function, and maintained as a []string.

Made it a set with 0e5e8a4

and use new cluster.Info API and constants Signed-off-by: Janki Chhatbar <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

.. and delete cobra stuff from internal/gather/gather.go Signed-off-by: Janki Chhatbar <[email protected]>

... instead of maps and use them directly as Cobra flags directly via StringSliceVar. Signed-off-by: Tom Pantelis <[email protected]>

skitt · 2022-05-03T15:58:53Z

cmd/subctl/diagnose.go

@@ -210,7 +210,7 @@ func clusterInfoFromKubeConfig(kubeConfig string) *cluster.Info {
 	clientProducer, err := client.NewProducerFromRestConfig(config.Config)
 	exit.OnErrorWithMessage(err, fmt.Sprintf("Error creating client producer for kubeconfig %q", kubeConfig))

-	clusterInfo, err := cluster.NewInfo("", clientProducer)
+	clusterInfo, err := cluster.NewInfo("", clientProducer, nil)


This is unfortunate, but let’s not block the PR — see #2042.

submariner-bot · 2022-05-03T16:26:08Z

🤖 Closed branches: [z_pr2007/Jaanki/refactor_gather]

Jaanki requested review from Oats87, skitt, sridhargaddam and tpantelis as code owners April 7, 2022 12:00

Jaanki self-assigned this Apr 7, 2022

Jaanki linked an issue Apr 7, 2022 that may be closed by this pull request

subctl should be refactored to a library, and make subctl the CLI of that library #1329

Closed

48 tasks

tpantelis reviewed Apr 8, 2022

View reviewed changes

Jaanki requested a review from tpantelis April 12, 2022 09:46

Jaanki commented Apr 14, 2022

View reviewed changes

pkg/cluster/info.go Show resolved Hide resolved

Jaanki requested a review from mkolesnik as a code owner April 22, 2022 08:47

Jaanki mentioned this pull request Apr 22, 2022

subctl should be refactored to a library, and make subctl the CLI of that library #1329

Closed

48 tasks

Jaanki marked this pull request as draft April 22, 2022 11:55

Jaanki marked this pull request as ready for review April 25, 2022 10:08

tpantelis requested changes Apr 25, 2022

View reviewed changes

skitt requested changes May 3, 2022

View reviewed changes

tpantelis removed a link to an issue May 3, 2022

subctl should be refactored to a library, and make subctl the CLI of that library #1329

Closed

48 tasks

tpantelis approved these changes May 3, 2022

View reviewed changes

Jaanki and others added 4 commits May 3, 2022 11:44

Add config to cluster.Info API

5b25484

and use new cluster.Info API and constants Signed-off-by: Janki Chhatbar <[email protected]>

Move pkg/subctl/cmd/gather -> internal/gather

9a057b7

Signed-off-by: Tom Pantelis <[email protected]>

Use internal/gather

859f930

.. and delete cobra stuff from internal/gather/gather.go Signed-off-by: Janki Chhatbar <[email protected]>

Use string slices for gather Types and Modules

8e66a3e

... instead of maps and use them directly as Cobra flags directly via StringSliceVar. Signed-off-by: Tom Pantelis <[email protected]>

skitt mentioned this pull request May 3, 2022

Encapsulate SPDY bidirectional upgrades in client.Producer #2042

Closed

skitt approved these changes May 3, 2022

View reviewed changes

submariner-bot added the ready-to-test When a PR is ready for full E2E testing label May 3, 2022

skitt enabled auto-merge (rebase) May 3, 2022 15:59

skitt merged commit 734ece2 into submariner-io:devel May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pkg/subctl/cmd/gather -> internal/gather #2007

Refactor pkg/subctl/cmd/gather -> internal/gather #2007

Jaanki commented Apr 7, 2022 •

edited by tpantelis

Loading

submariner-bot commented Apr 7, 2022

tpantelis left a comment

Jaanki commented Apr 8, 2022

tpantelis left a comment

dfarrell07 commented Apr 26, 2022

Jaanki commented Apr 27, 2022

tpantelis commented Apr 27, 2022 •

edited

Loading

skitt May 3, 2022

tpantelis May 3, 2022

skitt May 3, 2022

tpantelis May 3, 2022

skitt May 3, 2022

tpantelis May 3, 2022

skitt May 3, 2022

tpantelis May 3, 2022

submariner-bot commented May 3, 2022

Refactor pkg/subctl/cmd/gather -> internal/gather #2007

Refactor pkg/subctl/cmd/gather -> internal/gather #2007

Conversation

Jaanki commented Apr 7, 2022 • edited by tpantelis Loading

submariner-bot commented Apr 7, 2022

tpantelis left a comment

Choose a reason for hiding this comment

Jaanki commented Apr 8, 2022

tpantelis left a comment

Choose a reason for hiding this comment

dfarrell07 commented Apr 26, 2022

Jaanki commented Apr 27, 2022

tpantelis commented Apr 27, 2022 • 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

submariner-bot commented May 3, 2022

Jaanki commented Apr 7, 2022 •

edited by tpantelis

Loading

tpantelis commented Apr 27, 2022 •

edited

Loading