🐛 clusterctl to show config provider without setting env vars #2905

nader-ziada · 2020-04-14T01:25:32Z

What this PR does / why we need it:

the clusterctl config provider now can show the configuration without any env vars being set
change the code path so we don't have to replace variables as part of this informational command

Which issue(s) this PR fixes
Fixes #2876

Release Notes:

⚠️ ComponentsClient.Get method definition changed from func(string, string, string) (Components, error) to func(ComponentsOptions) (Components, error)
⚠️ NewComponents method definition changed from func(sigs.k8s.io/cluster-api/cmd/clusterctl/client/config.Provider, string, []byte, sigs.k8s.io/cluster-api/cmd/clusterctl/client/config.Client, string, string) (*components, error) to func(sigs.k8s.io/cluster-api/cmd/clusterctl/client/config.Provider, sigs.k8s.io/cluster-api/cmd/clusterctl/client/config.Client, []byte, ComponentsOptions) (*components, error)

nader-ziada · 2020-04-14T13:45:47Z

@wfernandes @JoelSpeed when you get a chance to review this, let me know what you think about which tests are missing. thanks

cmd/clusterctl/client/config_test.go

cmd/clusterctl/client/repository/components_client.go

cmd/clusterctl/client/config_test.go

cmd/clusterctl/client/common.go

vincepri

/approve
/assign @fabriziopandini
/milestone v0.3.4

k8s-ci-robot · 2020-04-14T15:33:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada, vincepri

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:

~~cmd/clusterctl/OWNERS~~ [vincepri]

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

wfernandes · 2020-04-14T16:37:03Z

cmd/clusterctl/client/repository/components.go

+// 3. Ensure all the ClusterRoleBinding which are referencing namespaced objects have the name prefixed with the namespace name
+// 4. Set the watching namespace for the provider controller
+// 5. Adds labels to all the components in order to allow easy identification of the provider objects
+func NewComponents(provider config.Provider, version string, rawyaml []byte, configClient config.Client, targetNamespace, watchingNamespace string) (*components, error) {


Suggestion: I'm wondering if we could refactor and collapse the two methods NewComponents and NewComponentsWithValues into one method takes a struct like ComponentsInput that contains all the required arguments and one more called replaceVariables bool. If replaceVariables is set, then we call replaceVariables(...).

I tried at first adding a bool like you are suggesting, but all kinds of tests broke because these functions are called from a bunch of places at different levels, so some places call replace variables directly and other places call NewComponents. It would have been a real big refactor

Hmm...yes it would be a sizeable change across the layers of clusterctl from getComponentsByName -> ComponentsClient.Get -> NewComponents.
But I'm leaning more towards that because currently the clusterctl API is subject to change regardless and I'd prefer not having extra methods in the interfaces that we may have to support in the future.
I'll let the others weigh in.
/cc @fabriziopandini

these are different methods that are used for different use cases, adding the bool won't change that, it would just make header to read

As per comment #2876 (comment), I don't see such a clear distinction between the two use cases because variable substitution could provide value to users also in clusterctl config cluster with the -o yaml option.

Said that I like the idea of avoiding duplicating methods, even if I fully understand the pain of fixing the chain of calls (I experimented also some refactoring to get rid of this, but this is invasive).

wfernandes · 2020-04-14T16:44:30Z

cmd/clusterctl/client/repository/components_client_test.go

@@ -234,7 +234,7 @@ func Test_componentsClient_Get(t *testing.T) {
 			gs := NewWithT(t)

 			f := newComponentsClient(tt.fields.provider, tt.fields.repository, configClient)
-			got, err := f.Get(tt.args.version, tt.args.targetNamespace, tt.args.watchingNamespace)


Changing this means that we don't have tests for Get. If we share the logic this might be fine, or we can add a test specifically for the Get without values? WDYT?

how about I refactor the func to share the common code so I don't just duplicate the tests?

wfernandes · 2020-04-14T16:47:45Z

cmd/clusterctl/client/repository/components_client.go

@@ -47,18 +48,53 @@ func newComponentsClient(provider config.Provider, repository Repository, config
 	}
 }

+func (f *componentsClient) GetWithValues(version, targetNamespace, watchingNamespace string) (Components, error) {


Suggestion:

Suggested change

func (f *componentsClient) GetWithValues(version, targetNamespace, watchingNamespace string) (Components, error) {

type ComponentsClientInput struct {

version, targetNamespace, watchingNamespace string

withValues bool // Feel free to choose a more appropriate name

}

func (f *componentsClient) Get(input ComponentsClientInput) (Components, error) {

}

Then we can pass in the withValues to NewComponents.

I also noticed that we are missing the godoc for these methods. Maybe we can add them in here now.

will add godoc

wfernandes · 2020-04-14T16:54:09Z

cmd/clusterctl/client/common.go

+// getComponentsByNameAndValue is a utility method that returns components
+// for a given provider, targetNamespace, and watchingNamespace.
+func (c *clusterctlClient) getComponentsByNameAndValue(provider string, providerType clusterctlv1.ProviderType, targetNamespace string, watchingNamespace string) (repository.Components, error) {


nit: The godoc for this is the same as getComponentsByName, can we differentiate them or say something like it is the same as getComponentsByName except for variables being filled in.

what if the user only looks at one of them? I can change it if you feel strongly about it :D

fabriziopandini

@nader-ziada sorry for jumping on this Issue with some delay
As per #2876 (comment), I'm personally for a slightly different solution, but I reviewed the PR as it is now

fabriziopandini · 2020-04-15T09:07:48Z

cmd/clusterctl/client/repository/components_client.go

+		return nil, err
+	}
+
+	return NewComponents(f.provider, version, file, f.configClient, targetNamespace, watchingNamespace)


Changing the behavior of an existing method should be clearly advised to the users.
Also, get in clusterctl Get is used for upgrades

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

Line 328 in 810240a

components, err := providerRepository.Components().Get(provider.NextVersion, provider.Namespace, provider.WatchedNamespace)

And this should be replaced with GetWithValues

fabriziopandini · 2020-04-15T09:16:24Z

cmd/clusterctl/client/repository/components.go

+// 3. Ensure all the ClusterRoleBinding which are referencing namespaced objects have the name prefixed with the namespace name
+// 4. Set the watching namespace for the provider controller
+// 5. Adds labels to all the components in order to allow easy identification of the provider objects
+func NewComponents(provider config.Provider, version string, rawyaml []byte, configClient config.Client, targetNamespace, watchingNamespace string) (*components, error) {


As per comment #2876 (comment), I don't see such a clear distinction between the two use cases because variable substitution could provide value to users also in clusterctl config cluster with the -o yaml option.

Said that I like the idea of avoiding duplicating methods, even if I fully understand the pain of fixing the chain of calls (I experimented also some refactoring to get rid of this, but this is invasive).

nader-ziada · 2020-04-16T14:35:45Z

@wfernandes @fabriziopandini added a 2nd commit with the flag approach, let me know what you think. Thanks

vincepri · 2020-04-16T16:31:44Z

/test pull-cluster-api-apidiff

nader-ziada · 2020-04-16T18:25:39Z

The api-apidiff is failing because I had to change a clusterctl method signature to fix this issue @vincepri

cmd/clusterctl/client/repository/components_client.go

wfernandes · 2020-04-16T20:10:09Z

This lgtm. Handing off to @fabriziopandini for final lgtm.

fabriziopandini

@nader-ziada thanks for this new version! really appreciated
Two nits and then lgtm from my side

fabriziopandini · 2020-04-17T15:47:25Z

cmd/clusterctl/client/client.go

-	// GetProviderComponents returns the provider components for a given provider, targetNamespace, watchingNamespace.
-	GetProviderComponents(provider string, providerType clusterctlv1.ProviderType, targetNameSpace, watchingNamespace string) (Components, error)
+	// GetProviderComponents returns the provider components for a given provider with options including targetNamespace, watchingNamespace.
+	GetProviderComponents(provider string, providerType clusterctlv1.ProviderType, options repository.ComponentsOptions) (Components, error)


Is it possible to define an alias for repository.ComponentsOptions in cmd/clusterctl/client/alias.go so we are not forcing to import the repository package to the consumer

Such alias should be used in all client the public methods

will add the alias

fabriziopandini · 2020-04-17T15:53:34Z

cmd/clusterctl/client/config.go

-func (c *clusterctlClient) GetProviderComponents(provider string, providerType clusterctlv1.ProviderType, targetNameSpace, watchingNamespace string) (Components, error) {
-	components, err := c.getComponentsByName(provider, providerType, targetNameSpace, watchingNamespace)
+func (c *clusterctlClient) GetProviderComponents(provider string, providerType clusterctlv1.ProviderType, options repository.ComponentsOptions) (Components, error) {
+	options.SkipVariables = true


Given that we are exponing options, should we leave to choose if to set this flag or not to the CLI/Library consumers?

Maybe this can be in a follow up PR

k8s-ci-robot · 2020-04-17T20:29:15Z

@nader-ziada: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff	`8103e29`	link	`/test pull-cluster-api-apidiff`

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.

fabriziopandini · 2020-04-17T21:40:45Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 14, 2020

k8s-ci-robot requested review from JoelSpeed and wfernandes April 14, 2020 01:25

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2020

vincepri reviewed Apr 14, 2020

View reviewed changes

nader-ziada force-pushed the config-provider branch from ae41205 to 5c2df9f Compare April 14, 2020 15:23

vincepri approved these changes Apr 14, 2020

View reviewed changes

k8s-ci-robot assigned fabriziopandini Apr 14, 2020

k8s-ci-robot added this to the v0.3.4 milestone Apr 14, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020

wfernandes reviewed Apr 14, 2020

View reviewed changes

nader-ziada force-pushed the config-provider branch 2 times, most recently from d8fa0cc to 7fae5de Compare April 14, 2020 18:12

fabriziopandini requested changes Apr 15, 2020

View reviewed changes

nader-ziada force-pushed the config-provider branch from 7fae5de to 1fd9cea Compare April 16, 2020 14:34

nader-ziada force-pushed the config-provider branch from 1fd9cea to ee99878 Compare April 16, 2020 14:44

wfernandes reviewed Apr 16, 2020

View reviewed changes

cmd/clusterctl/client/repository/components_client.go Outdated Show resolved Hide resolved

nader-ziada force-pushed the config-provider branch from ee99878 to f2013dd Compare April 17, 2020 14:18

fabriziopandini reviewed Apr 17, 2020

View reviewed changes

nader-ziada force-pushed the config-provider branch from f2013dd to 79df486 Compare April 17, 2020 20:19

use skipvariables flag for config provider cmd

8103e29

nader-ziada force-pushed the config-provider branch from 79df486 to 8103e29 Compare April 17, 2020 20:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2020

k8s-ci-robot merged commit 62b7a1d into kubernetes-sigs:master Apr 17, 2020

fabriziopandini mentioned this pull request Apr 20, 2020

🏃Clusterctl config small improvements #2935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 clusterctl to show config provider without setting env vars #2905

🐛 clusterctl to show config provider without setting env vars #2905

nader-ziada commented Apr 14, 2020 •

edited

Loading

nader-ziada commented Apr 14, 2020

vincepri left a comment

k8s-ci-robot commented Apr 14, 2020

wfernandes Apr 14, 2020

nader-ziada Apr 14, 2020

wfernandes Apr 14, 2020 •

edited

Loading

nader-ziada Apr 15, 2020

fabriziopandini Apr 15, 2020

wfernandes Apr 14, 2020

nader-ziada Apr 14, 2020

wfernandes Apr 14, 2020

wfernandes Apr 14, 2020

nader-ziada Apr 14, 2020

wfernandes Apr 14, 2020

nader-ziada Apr 14, 2020

fabriziopandini left a comment

fabriziopandini Apr 15, 2020

fabriziopandini Apr 15, 2020

nader-ziada commented Apr 16, 2020

vincepri commented Apr 16, 2020

nader-ziada commented Apr 16, 2020

wfernandes commented Apr 16, 2020

fabriziopandini left a comment

fabriziopandini Apr 17, 2020

nader-ziada Apr 17, 2020

fabriziopandini Apr 17, 2020

nader-ziada Apr 17, 2020

k8s-ci-robot commented Apr 17, 2020

fabriziopandini commented Apr 17, 2020

-func (f *componentsClient) GetWithValues(version, targetNamespace, watchingNamespace string) (Components, error) {
+type ComponentsClientInput struct {
+  version, targetNamespace, watchingNamespace string
+  withValues bool // Feel free to choose a more appropriate name
+}
+func (f *componentsClient) Get(input ComponentsClientInput) (Components, error) {
+}

🐛 clusterctl to show config provider without setting env vars #2905

🐛 clusterctl to show config provider without setting env vars #2905

Conversation

nader-ziada commented Apr 14, 2020 • edited Loading

nader-ziada commented Apr 14, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfernandes Apr 14, 2020 • 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

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Apr 16, 2020

vincepri commented Apr 16, 2020

nader-ziada commented Apr 16, 2020

wfernandes commented Apr 16, 2020

fabriziopandini 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

k8s-ci-robot commented Apr 17, 2020

fabriziopandini commented Apr 17, 2020

nader-ziada commented Apr 14, 2020 •

edited

Loading

wfernandes Apr 14, 2020 •

edited

Loading