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

Use named custom func #71447

Open
wants to merge 2 commits into
base: master
from

Conversation

@dtaniwaki
Contributor

dtaniwaki commented Nov 27, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:

cobra can handle a custom function name for __custom_func as of spf13/cobra@6fd8e29. We should use it to avoid conflictions with other command line clients w/ the cobra's completion.

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

N/A

Special notes for your reviewer:

I did the following steps to update the cobra revision.

$ hack/run-in-gopath.sh bash -c 'rm -rf $GOPATH/src/github.com/spf13/cobra'
$ hack/run-in-gopath.sh go get github.com/mitchellh/go-homedir
$ hack/run-in-gopath.sh go get -d github.com/spf13/cobra
$ hack/run-in-gopath.sh bash -c 'git -C $GOPATH/src/github.com/spf13/cobra checkout 6fd8e29'
$ hack/run-in-gopath.sh hack/godep-save.sh

Does this PR introduce a user-facing change?:

Avoid __custom_func conflictions with other cobra completions.
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 27, 2018

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

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Nov 27, 2018

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:use-named-custom-func branch 2 times, most recently from ea60908 to e682e82 Nov 27, 2018

@cblecker

This comment has been minimized.

Member

cblecker commented Dec 3, 2018

/ok-to-test
/priority backlog
/cc @kubernetes/sig-cli-maintainers @eparis

This should be reviewed by sig-cil first. Dep bump looks okay.

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Dec 3, 2018

/retest

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:use-named-custom-func branch from 66a248f to 556e0b4 Dec 3, 2018

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Dec 3, 2018

/retest

@cblecker

This comment has been minimized.

Member

cblecker commented Dec 3, 2018

You can’t manually modify vendor/. CI failure is legitimate.

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:use-named-custom-func branch from 556e0b4 to c0b04ec Dec 6, 2018

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Dec 6, 2018

hack/verify-staging-godeps.sh, which failed in the CI, succeeds in my local. I will retry the tests.

/retest

@cblecker

This comment has been minimized.

Member

cblecker commented Dec 6, 2018

@dtaniwaki Try rebasing on master, and rerunning hack/update-staging-godeps.sh.

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:use-named-custom-func branch from c0b04ec to 5f7c95b Dec 7, 2018

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Dec 7, 2018

I realized I haven't updated the dependencies in staging by ./hack/update-staging-godeps.sh. Will do.

dtaniwaki added some commits Dec 7, 2018

@dtaniwaki dtaniwaki force-pushed the dtaniwaki:use-named-custom-func branch from 5f7c95b to 511d23c Dec 7, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 7, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dtaniwaki
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: brendandburns

If they are not already assigned, you can assign the PR to them by writing /assign @brendandburns in a comment when ready.

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

@dtaniwaki

This comment has been minimized.

Contributor

dtaniwaki commented Dec 7, 2018

@cblecker Thanks for your help. Finally, the CI succeeded!

@cblecker

This comment has been minimized.

Member

cblecker commented Dec 7, 2018

Looks like this is ready for sig-cli review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment