Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Custom Resource support to "kubectl autoscale" #72678

Merged
merged 1 commit into from Feb 26, 2019

Conversation

@rmohr
Copy link
Contributor

rmohr commented Jan 8, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Generalize the autoscsale command to simply let the dynamic client check
if a scale subresource is registered for the supplied type. This allows
using the autoscale command for built in types as well as for custom
resources.

Which issue(s) this PR fixes:

Fixes #72646

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add Custom Resource support to "kubectl autoscale"
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 8, 2019

Hi @rmohr. Thanks for your PR.

I'm waiting for a kubernetes 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.

@rmohr

This comment has been minimized.

Copy link
Contributor Author

rmohr commented Jan 8, 2019

/area custom-resources
/sig cli
/sig autoscaling

@rmohr

This comment has been minimized.

Copy link
Contributor Author

rmohr commented Jan 8, 2019

/cc @nikhita @sttts

would you be so kind to have a look on this one?

@rmohr

This comment has been minimized.

Copy link
Contributor Author

rmohr commented Jan 9, 2019

/cc @seans3

@k8s-ci-robot k8s-ci-robot requested a review from seans3 Jan 9, 2019

)

func TestCanBeAutoscaled(t *testing.T) {
func TestCoreTypesCanBeAutoscaled(t *testing.T) {

This comment has been minimized.

@Rajat-0

Rajat-0 Jan 11, 2019

why do we need to change the function name here?

This comment has been minimized.

@rmohr

rmohr Jan 11, 2019

Author Contributor

I added a second test case. In one I check for the core types in the other one for the non-core types. I can also combine it, if that is preferred.

This comment has been minimized.

@Rajat-0

Rajat-0 Jan 18, 2019

My doubt was that test case name according to Golang conventions should be TestFunctionName

@@ -36,7 +39,11 @@ func canBeAutoscaled(kind schema.GroupKind) error {
extensionsv1beta1.SchemeGroupVersion.WithKind("ReplicaSet").GroupKind():
// nothing to do here
default:
return fmt.Errorf("cannot autoscale a %v", kind)
gr := mapping.GroupVersionKind.GroupVersion().WithResource(mapping.Resource.Resource).GroupResource()
_, err := scaler.Scales(info.Namespace).Get(gr, info.Name)

This comment has been minimized.

@sttts

sttts Jan 11, 2019

Contributor

Discovery information has this as well. Can't we drop all the special case above and just depend on that?

This comment has been minimized.

@sttts

This comment has been minimized.

@DirectXMan12

DirectXMan12 Jan 14, 2019

Contributor

yes, we should be able to

This comment has been minimized.

@rmohr

rmohr Jan 18, 2019

Author Contributor

Switched to the dynamic client. Does it look like what you had in mind?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 11, 2019

@sttts: GitHub didn't allow me to request PR reviews from the following users: s-urbaniak.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @s-urbaniak @DirectXMan12

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.

@k8s-ci-robot k8s-ci-robot requested a review from DirectXMan12 Jan 11, 2019

@rmohr rmohr force-pushed the rmohr:cr-autoscale branch from 905205f to 24ea89c Jan 23, 2019

@rmohr

This comment has been minimized.

Copy link
Contributor Author

rmohr commented Jan 23, 2019

@sttts @DirectXMan12 switched to the dynamic client. Let me know if you had something else in mind.

@DirectXMan12
Copy link
Contributor

DirectXMan12 left a comment

minor nit inline, otherwise looks good

Show resolved Hide resolved pkg/kubectl/cmd/autoscale/autoscale.go Outdated

@rmohr rmohr force-pushed the rmohr:cr-autoscale branch from 24ea89c to 68e18fb Jan 30, 2019

Add Custom Resource support to "kubectl autoscale"
Generalize the autoscsale command to simply let the dynamic client check
if a scale subresource is registered for the supplied type. This allows
using the autoscale command for built in types as well as custom
resources.

@rmohr rmohr force-pushed the rmohr:cr-autoscale branch from 68e18fb to c947999 Feb 7, 2019

@rmohr

This comment has been minimized.

Copy link
Contributor Author

rmohr commented Feb 10, 2019

Let me know if anything else is missing to whitelist it for testing.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 11, 2019

/ok-to-test

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 11, 2019

/assign @soltysh

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm
/approve
Thanks for improving this code 👍 and sorry for delay reviewing 😊

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 26, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr, soltysh

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:

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 k8s-ci-robot merged commit fc1e528 into kubernetes:master Feb 26, 2019

17 checks passed

cla/linuxfoundation rmohr authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Context retired without replacement.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.