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

update vendor spf13/cobra to enforce required flags #53631

Merged
merged 3 commits into from
Jan 18, 2018
Merged

update vendor spf13/cobra to enforce required flags #53631

merged 3 commits into from
Jan 18, 2018

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Oct 10, 2017 •
edited
Loading

What this PR does / why we need it:

spf13/cobra#502 has enforced checking flags that marked as required, an error will be raised if unset.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):fixes #54855
xref #48400
fixes kubernetes/kubectl#121

Special notes for your reviewer:
/assign @liggitt @eparis

Release note:

kubectl now enforces required flags at a more fundamental level

@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/XS Denotes a PR that changes 0-9 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 Oct 10, 2017
@eparis
Copy link
Contributor

eparis commented Oct 10, 2017

/ok-to-test
I thought we also needed to bump pflag...

@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 Oct 10, 2017
@eparis
Copy link
Contributor

eparis commented Oct 10, 2017

CI says I'm right and we need to bump pflag as well.

@dixudx
Copy link
Member Author

dixudx commented Oct 11, 2017 •
edited
Loading

@eparis func (c *Command) Name() has been changed in spf13/cobra#390. This is causing deprecated alias not working as expected.

@dixudx dixudx force-pushed the enforce_cobra_required_flags branch from f2615c2 to 4e6c368 Compare October 11, 2017 06:27
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 11, 2017
@dixudx dixudx force-pushed the enforce_cobra_required_flags branch from 4e6c368 to b908fcf Compare October 11, 2017 07:41
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 11, 2017
@dixudx dixudx force-pushed the enforce_cobra_required_flags branch 2 times, most recently from 54c918f to aeb2e7e Compare October 12, 2017 03:25
@dixudx dixudx mentioned this pull request Nov 6, 2017
Closed
@dixudx
Copy link
Member Author

dixudx commented Nov 6, 2017

ping @liggitt @smarterclayton @kubernetes/sig-cli-maintainers PTAL. Thanks.

@yuexiao-wang
Copy link
Contributor

@dixudx Could you tell me how to update spf13/cobra ? Because I have some problem to update it, pleae show the detail steps. Thanks

@dixudx
Copy link
Member Author

dixudx commented Nov 6, 2017

@yuexiao-wang The bugs you mentioned in #54855 has been fixed in this PR. So there is no need to submit another PR.

how to update spf13/cobra

Kubernetes uses godep to manage all the deps. Here is the steps you can follow if you have other vendor/deps to update.

# step 1, restore all deps
$ ./hack/godep-restore.sh
# step2, update your dep in your GOPATH
$ <update local dep>
# step3, save the deps
$ ./hack/godep-save.sh

@yuexiao-wang
Copy link
Contributor

Got it and thanks

@eparis
Copy link
Contributor

eparis commented Nov 6, 2017

/lgtm

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

eparis commented Nov 6, 2017

/assign @lavalamp
/assign @smarterclayton

@dixudx dixudx force-pushed the enforce_cobra_required_flags branch from 36d74e2 to 3d858f2 Compare November 8, 2017 02:36
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 8, 2017
@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@liggitt liggitt assigned thockin and zjj2wry and unassigned sttts and ericchiang Jan 16, 2018
@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

1 similar comment
@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@jbeda
Copy link
Contributor

jbeda commented Jan 17, 2018 •
edited
Loading

I'll do the approval here if others aren't around. Looks totally sane.

I'd like to see the release note updated to something that users will actually be able to understand. Something like "kubectl now enforces required flags at a more fundamental level".

If you need to ping me feel free to reach out on slack or twitter as github notifications are super noisy for me.

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

soltysh
soltysh approved these changes Jan 17, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@soltysh
Copy link
Contributor

soltysh commented Jan 17, 2018

@jbeda || @thockin it's all your for approval

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@jbeda
Copy link
Contributor

jbeda commented Jan 17, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, eparis, jbeda, shiywang, soltysh, zjj2wry

Associated issue: #502

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2018
@jbeda
Copy link
Contributor

jbeda commented Jan 17, 2018

/test pull-kubernetes-e2e-kops-aws

@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.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@dixudx
Copy link
Member Author

dixudx commented Jan 18, 2018

/retest

1 similar comment
@dixudx
Copy link
Member Author

dixudx commented Jan 18, 2018

/retest

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@dixudx
Copy link
Member Author

dixudx commented Jan 18, 2018

/retest

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 048757b into kubernetes:master Jan 18, 2018
@dixudx dixudx deleted the enforce_cobra_required_flags branch January 18, 2018 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiywang shiywang shiywang left review comments

@soltysh soltysh soltysh approved these changes

@thockin thockin Awaiting requested review from thockin

Assignees

@soltysh soltysh

@thockin thockin

@zjj2wry zjj2wry

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. 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.

Autocomplete issues with --from-file and --from-literal Deprecated usage message is not getting printed