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.

Sign up for GitHub

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

Jump to bottom

Use structured generator for kubectl autoscale #55913

Merged
merged 2 commits into from
Dec 14, 2017
Merged

Use structured generator for kubectl autoscale #55913

merged 2 commits into from
Dec 14, 2017

Conversation

wackxu
Copy link
Contributor

@wackxu wackxu commented Nov 17, 2017 •
edited
Loading

What this PR does / why we need it:

Use structured generator for kubectl autoscale

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of # kubernetes/kubectl#138

Special notes for your reviewer:
/assign @mengqiy

Release note:

Use structured generator for kubectl autoscale

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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 17, 2017
@zjj2wry
Copy link
Contributor

zjj2wry commented Nov 17, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 17, 2017
zjj2wry
zjj2wry reviewed Nov 17, 2017
View reviewed changes
pkg/kubectl/autoscale.go Outdated
return fmt.Errorf("name must be specified")
}
if s.MaxReplicas <= 0 {
return fmt.Errorf("'max' is a required parameter and must be greater than zero")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this different with validateFlags func in autoscale.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zjj2wry Thanks for your review, Done, PTAL

@wackxu wackxu force-pushed the refauto branch 2 times, most recently from e47d5bb to 53b9e51 Compare November 17, 2017 05:33
@wackxu
Copy link
Contributor Author

wackxu commented Nov 17, 2017

/test pull-kubernetes-verify

@wackxu
Copy link
Contributor Author

wackxu commented Nov 17, 2017

/test pull-kubernetes-unit

@mengqiy
Copy link
Member

mengqiy commented Nov 18, 2017

I suggest you split this PR into 2 PR.
Merge the refactoring half first. This half LG.
Then the kubectl create half.

@wackxu wackxu changed the title Use structured generator for kubectl autoscale and add kubectl create hpa command Use structured generator for kubectl autoscale Nov 20, 2017
@wackxu
Copy link
Contributor Author

wackxu commented Nov 20, 2017

/retest

@wackxu
Copy link
Contributor Author

wackxu commented Nov 20, 2017

@mengqiy Done, PTAL

@wackxu
Copy link
Contributor Author

wackxu commented Nov 20, 2017

/test pull-kubernetes-e2e-gce

@mengqiy
Copy link
Member

mengqiy commented Nov 20, 2017

/lgtm

/assign @fabianofranz
Can you approve this?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2017
@fabianofranz
Copy link
Contributor

Thanks @wackxu!
/approve
/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Nov 22, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianofranz, mengqiy, wackxu

Associated issue: 138

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55900, 55995, 55913, 55467, 55376). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 8e16121 into kubernetes:master Dec 14, 2017
@wackxu wackxu deleted the refauto branch January 12, 2018 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zjj2wry zjj2wry zjj2wry left review comments

Assignees

@fabianofranz fabianofranz

@mengqiy mengqiy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wackxu @zjj2wry @mengqiy @fabianofranz @k8s-github-robot @fejta-bot @k8s-ci-robot