Fixes 2932: let the capi version to be discovered #2934

enxebre · 2020-03-13T12:37:24Z

Add the ability to override CAPI group via env variable and discovers the API version.

This change adds detection for an environment variable to specify the group for the clusterapi resources. If the environment variable CAPI_GROUP is specified, then it will be used instead of the default. This also decouples the API group from the version and let the latter to be discovered dynamically.

Fixes #2932

enxebre · 2020-03-13T12:38:04Z

/hold
To do some manual validations
cc @frobware @elmiko

enxebre · 2020-03-13T15:11:56Z

/hold cancel

JoelSpeed

Codewise looks good, just wondering whether we want to be calling all the variables etc Machine API or Cluster API, do we have a preference?

elmiko · 2020-03-13T18:14:40Z

Codewise looks good, just wondering whether we want to be calling all the variables etc Machine API or Cluster API, do we have a preference?

agreed, i think if we can use "cluster api" here it will reduce any confusion for someone reading this. at the very least we should name the external bits (eg env variable) to reflect cluster api.

frobware · 2020-03-16T09:25:28Z

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go


-	machineSetResource, _ := schema.ParseResourceArg(fmt.Sprintf("machinesets.%v", defaultMachineAPI))
+	machineSetResource, _ := schema.ParseResourceArg(fmt.Sprintf("machinesets.%v", specifiedMachineAPI))
 	if machineSetResource == nil {
 		panic("MachineSetResource")


This should really return an error.

mm why? atm MachineDeployment, MachineSets and Machine` are intentionally assumed to exist so it seems reasonable to me to panic here?

Because it makes unit tests easier to write and assert for expected behaviour.

ok make sense, let's address this separately from this PR. I'll move the capi resource expectations into its own unit tested func which errors and let the caller panic.

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

enxebre · 2020-03-16T09:45:59Z

Codewise looks good, just wondering whether we want to be calling all the variables etc Machine API or Cluster API, do we have a preference?

agreed, i think if we can use "cluster api" here it will reduce any confusion for someone reading this. at the very least we should name the external bits (eg env variable) to reflect cluster api.

I renamed to CAPI consistently and moved code to fetch the preferred version into unit tested getAPIGroupPreferredVersion

elmiko

in general it looks good to me, deduplicating the tests seems like a good idea.

enxebre · 2020-03-16T13:01:07Z

in general it looks good to me, deduplicating the tests seems like a good idea.

Addressed comments and meld into one commit. PTAL @elmiko @frobware @JoelSpeed

elmiko

/lgtm

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go

…API version. This change adds detection for an environment variable to specify the group for the clusterapi resources. If the environment variable `CAPI_GROUP` is specified, then it will be used instead of the default. This also decouples the API group from the version and let the latter to be discovered dynamically.

frobware

The issue (#2934) talks about discovering the version but, unless I'm mistaken, this only allows the group to be overridden. True?

enxebre · 2020-03-16T14:09:48Z

The issue (#2934) talks about discovering the version but, unless I'm mistaken, this only allows the group to be overridden. True?

This PR enables an env variable to override the default CAPI group and discovers the API version (#2932) see https://github.com/kubernetes/autoscaler/pull/2934/files#diff-89363471d762322b93c9bcfec57b5af8R302. Let me update the PR desc with the commit message.

frobware · 2020-03-16T15:36:02Z

/approve

k8s-ci-robot · 2020-03-16T15:36:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware

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:

~~cluster-autoscaler/cloudprovider/clusterapi/OWNERS~~ [frobware]

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

enxebre · 2020-03-16T15:38:28Z

/area provider/cluster-api

elmiko · 2020-03-16T15:57:41Z

/lgtm

[CA-1.18] #2934 cherry-pick: Fixes 2932: let the capi version to be discovered

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 13, 2020

k8s-ci-robot requested review from elmiko and hardikdr March 13, 2020 12:37

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2020

JoelSpeed reviewed Mar 13, 2020

View reviewed changes

frobware reviewed Mar 16, 2020

View reviewed changes

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go Outdated Show resolved Hide resolved

frobware reviewed Mar 16, 2020

View reviewed changes

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go Show resolved Hide resolved

enxebre force-pushed the fix-2932 branch 2 times, most recently from 2f20344 to 223ef38 Compare March 16, 2020 09:43

elmiko reviewed Mar 16, 2020

View reviewed changes

enxebre force-pushed the fix-2932 branch from 223ef38 to 8495662 Compare March 16, 2020 12:58

elmiko reviewed Mar 16, 2020

View reviewed changes

k8s-ci-robot assigned elmiko Mar 16, 2020

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

frobware reviewed Mar 16, 2020

View reviewed changes

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go Outdated Show resolved Hide resolved

enxebre force-pushed the fix-2932 branch from 8495662 to 33d8c0e Compare March 16, 2020 13:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2020

frobware reviewed Mar 16, 2020

View reviewed changes

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller.go Outdated Show resolved Hide resolved

frobware reviewed Mar 16, 2020

View reviewed changes

cluster-autoscaler/cloudprovider/clusterapi/clusterapi_controller_test.go Outdated Show resolved Hide resolved

enxebre force-pushed the fix-2932 branch from 33d8c0e to 0f4a808 Compare March 16, 2020 13:58

enxebre force-pushed the fix-2932 branch from 0f4a808 to 7082cfe Compare March 16, 2020 13:59

frobware reviewed Mar 16, 2020

View reviewed changes

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

k8s-ci-robot added the area/provider/cluster-api Issues or PRs related to Cluster API provider label Mar 16, 2020

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

k8s-ci-robot merged commit ee627f2 into kubernetes:master Mar 16, 2020

enxebre mentioned this pull request Mar 17, 2020

Rebase 1.18 openshift/kubernetes-autoscaler#139

Merged

kron4eg mentioned this pull request Apr 28, 2020

Integrate cluster-autoscaler as addon kubermatic/kubeone#391

Closed

xmudrii mentioned this pull request Apr 30, 2020

[CA-1.18] #2934 cherry-pick: Fixes 2932: let the capi version to be discovered #3105

Merged

k8s-ci-robot added a commit that referenced this pull request May 4, 2020

Merge pull request #3105 from xmudrii/cherry-pick-2934

9cd7d97

[CA-1.18] #2934 cherry-pick: Fixes 2932: let the capi version to be discovered

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 2932: let the capi version to be discovered #2934

Fixes 2932: let the capi version to be discovered #2934

enxebre commented Mar 13, 2020 •

edited

Loading

enxebre commented Mar 13, 2020

enxebre commented Mar 13, 2020

JoelSpeed left a comment

elmiko commented Mar 13, 2020

frobware Mar 16, 2020

enxebre Mar 16, 2020

frobware Mar 16, 2020

enxebre Mar 16, 2020 •

edited

Loading

enxebre commented Mar 16, 2020 •

edited

Loading

elmiko left a comment

enxebre commented Mar 16, 2020

elmiko left a comment

frobware left a comment •

edited

Loading

enxebre commented Mar 16, 2020 •

edited

Loading

frobware commented Mar 16, 2020

k8s-ci-robot commented Mar 16, 2020

enxebre commented Mar 16, 2020

elmiko commented Mar 16, 2020

Fixes 2932: let the capi version to be discovered #2934

Fixes 2932: let the capi version to be discovered #2934

Conversation

enxebre commented Mar 13, 2020 • edited Loading

enxebre commented Mar 13, 2020

enxebre commented Mar 13, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

elmiko commented Mar 13, 2020

frobware Mar 16, 2020

Choose a reason for hiding this comment

enxebre Mar 16, 2020

Choose a reason for hiding this comment

frobware Mar 16, 2020

Choose a reason for hiding this comment

enxebre Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

enxebre commented Mar 16, 2020 • edited Loading

elmiko left a comment

Choose a reason for hiding this comment

enxebre commented Mar 16, 2020

elmiko left a comment

Choose a reason for hiding this comment

frobware left a comment • edited Loading

Choose a reason for hiding this comment

enxebre commented Mar 16, 2020 • edited Loading

frobware commented Mar 16, 2020

k8s-ci-robot commented Mar 16, 2020

enxebre commented Mar 16, 2020

elmiko commented Mar 16, 2020

enxebre commented Mar 13, 2020 •

edited

Loading

enxebre Mar 16, 2020 •

edited

Loading

enxebre commented Mar 16, 2020 •

edited

Loading

frobware left a comment •

edited

Loading

enxebre commented Mar 16, 2020 •

edited

Loading