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 cobra dependency to v1.1.1 #95571

Merged
merged 1 commit into from Nov 5, 2020
Merged

Update cobra dependency to v1.1.1 #95571

merged 1 commit into from Nov 5, 2020

Conversation

eddiezane
Copy link
Member

@eddiezane eddiezane commented Oct 14, 2020 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR bumps the vendored version of cobra to v1.1.1.

This version includes some changes to shell completion necessary for ongoing kubectl work.

Which issue(s) this PR fixes:

Refs kubernetes/kubectl#882

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @marckhouzam @brianpursley @knight42

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Oct 14, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 14, 2020
@k8s-ci-robot
Copy link
Contributor

@eddiezane: GitHub didn't allow me to request PR reviews from the following users: marckhouzam.

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

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR bumps the vendored version of cobra to v1.1.0.

This version includes some changes to shell completion necessary for ongoing kubectl work.

Which issue(s) this PR fixes:

Refs kubernetes/kubectl#882

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @marckhouzam @brianpursley @knight42

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 14, 2020
@eddiezane eddiezane mentioned this pull request Oct 14, 2020
Closed
@eddiezane
Copy link
Member Author

eddiezane commented Oct 14, 2020 •
edited

Notes for reviewers: I've followed this guide on updating. I'm not sure if the gopkg.in/yaml.v2 changes are correct?

@eddiezane
Copy link
Member Author

/assign @liggitt

liggitt
liggitt reviewed Oct 14, 2020
View reviewed changes
go.mod Show resolved Hide resolved
go.mod Outdated
@@ -109,7 +109,7 @@ require (
google.golang.org/grpc v1.27.0
gopkg.in/gcfg.v1 v1.2.0
gopkg.in/square/go-jose.v2 v2.2.2
gopkg.in/yaml.v2 v2.2.8
gopkg.in/yaml.v2 v2.3.0
Copy link
Member

Choose a reason for hiding this comment

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

running hack/lint-dependencies.sh will indicate this needs to be updated and vendor updated

Copy link
Member

Choose a reason for hiding this comment

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

looks like this version changed the default line width, so we may need to explicitly set the previous default of 80 in our yaml serializers

Copy link
Member Author

@eddiezane eddiezane Oct 14, 2020 •
edited

Choose a reason for hiding this comment

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

This is the related change go-yaml/yaml#571 but I'm not sure where to start digging in on our end outside of seeing what the tests do.

Copy link
Member Author

Choose a reason for hiding this comment

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

And there are the failures. I'll dig in.

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/95571/pull-kubernetes-bazel-test/1316503504432402432

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2020
@eddiezane eddiezane force-pushed the ez/update-cobra branch 2 times, most recently from 6ac6688 to 9e81dea Compare November 2, 2020 22:19
@eddiezane
Update cobra dependency to v1.1.1
9b65bd8 
Cobra v1.1.1 brings improvements to autocompletion needed for ongoing kubectl work.

Signed-off-by: Eddie Zaneski <eddiezane@gmail.com>
@eddiezane
Copy link
Member Author

@liggitt - rebased and ready to go

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 2, 2020
soltysh
soltysh approved these changes Nov 3, 2020
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
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2020
@dims
Copy link
Member

dims commented Nov 3, 2020

looks like this is ready to 🚢 @liggitt (needs root OWNER approval)

@soltysh
Copy link
Contributor

soltysh commented Nov 3, 2020

Good point Dims, worth
/hold cancel
too ;)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2020
@eddiezane
Copy link
Member Author

eddiezane commented Nov 4, 2020 •
edited

@liggitt per Slack here's the list of new entries to go.sum with their license.

Module License
cloud.google.com/go/firestore Apache-2.0
github.com/armon/go-metrics MIT
github.com/armon/go-radix MIT
github.com/bketelsen/crypt MIT
github.com/coreos/bbolt MIT
github.com/coreos/etcd Apache-2.0
github.com/gorilla/websocket BSD-2-Clause
github.com/hashicorp/consul/api MPL-2.0
github.com/hashicorp/consul/sdk MPL-2.0
github.com/hashicorp/errwrap MPL-2.0
github.com/hashicorp/go-cleanhttp MPL-2.0
github.com/hashicorp/go-immutable-radix MPL-2.0
github.com/hashicorp/go-msgpack BSD-3-Clause
github.com/hashicorp/go-multierror MPL-2.0
github.com/hashicorp/go-rootcerts MPL-2.0
github.com/hashicorp/go-sockaddr MPL-2.0
github.com/hashicorp/go-uuid MPL-2.0
github.com/hashicorp/go.net Golang Standard License (https://github.com/hashicorp/go.net/blob/master/LICENSE)
github.com/hashicorp/logutils MPL-2.0
github.com/hashicorp/mdns MIT
github.com/hashicorp/memberlist MPL-2.0
github.com/hashicorp/serf MPL-2.0
github.com/mitchellh/cli MPL-2.0
github.com/mitchellh/go-testing-interface MIT
github.com/mitchellh/gox MPL-2.0
github.com/mitchellh/iochan MIT
github.com/pascaldekloe/goe CC0 1.0 Universal (https://github.com/pascaldekloe/goe/blob/master/LICENSE https://creativecommons.org/publicdomain/zero/1.0/legalcode)
github.com/posener/complete MIT
github.com/ryanuber/columnize MIT
github.com/sean-/seed MIT + Golang Standard License (https://github.com/sean-/seed/blob/master/LICENSE)
github.com/spf13/cobra Apache-2.0
github.com/spf13/viper MIT
github.com/subosito/gotenv MIT
gopkg.in/ini.v1 Apache-2.0

@liggitt
Copy link
Member

liggitt commented Nov 5, 2020

Thanks for gathering that. MPL and CC0 are not on the allowed list of CNCF licenses, and I don't see the following in https://github.com/cncf/foundation/tree/master/license-exceptions:

  • github.com/pascaldekloe/goe
  • github.com/hashicorp/consul
  • github.com/hashicorp/go-immutable-radix
  • github.com/hashicorp/go-msgpack
  • github.com/hashicorp/go-rootcerts
  • github.com/hashicorp/go-sockaddr
  • github.com/hashicorp/go-uuid
  • github.com/hashicorp/logutils
  • github.com/hashicorp/memberlist
  • github.com/hashicorp/serf
  • github.com/mitchellh/cli
  • github.com/mitchellh/gox

@eddiezane
Copy link
Member Author

This was a fun one.

open_SAKnExcG

It looks like all of these are pulled in via: github.com/spf13/cobra -> github.com/spf13/viper -> github.com/bketelsen/crypt -> github.com/hashicorp/consul/api

Seeing as this version of cobra is already in etcd I imagine we'll need to get these on the exception list. Do you know what the path for that is?

@liggitt
Copy link
Member

liggitt commented Nov 5, 2020

I don't... @cblecker, do you know?

@liggitt
Copy link
Member

liggitt commented Nov 5, 2020

cc @kubernetes/licensing

@dims
Copy link
Member

dims commented Nov 5, 2020

@liggitt @eddiezane So what really matters is the LICENSES file that get added, not transitive, so for this PR those are

 create mode 100644 LICENSES/vendor/github.com/subosito/gotenv/LICENSE
 create mode 100644 LICENSES/vendor/gopkg.in/ini.v1/LICENSE

Given the both the packages have licenses that are covered by https://github.com/cncf/foundation/blob/master/allowed-third-party-license-policy.md#approved-licenses-for-allowlist

github.com/subosito/gotenv | MIT
gopkg.in/ini.v1            | Apache-2.0

We should be good.

Also, process-wise, the exception-list in CNCF lags what we release. basically we get to ask for exceptions as and when we need it. The CNCF GB approves/updates the exception list. We ping @swinslow when we need to ask questions. Typically @nikhita and myself have engaged Steve.

Thanks,
Dims

@dims
Copy link
Member

dims commented Nov 5, 2020

/approve
/lgtm

@liggitt
Copy link
Member

liggitt commented Nov 5, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, eddiezane, liggitt, 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
@k8s-ci-robot k8s-ci-robot merged commit 11608f8 into kubernetes:master Nov 5, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 5, 2020
@eddiezane eddiezane deleted the ez/update-cobra branch November 5, 2020 17:40
@bobbypage bobbypage mentioned this pull request Nov 12, 2020
Merged
@dims dims mentioned this pull request Nov 12, 2020
Closed
@mkurde mkurde mentioned this pull request Oct 14, 2021
Closed
@eddiezane eddiezane mentioned this pull request Apr 12, 2022
Closed
@BenTheElder BenTheElder mentioned this pull request May 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@brianpursley brianpursley Awaiting requested review from brianpursley

@knight42 knight42 Awaiting requested review from knight42

@liggitt liggitt Awaiting requested review from liggitt

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@dims dims

@soltysh soltysh

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/code-organization Issues or PRs related to kubernetes code organization area/dependency Issues or PRs related to dependency changes area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet