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

Move cloud-controller-manager to staging k8s.io/cloud-provider #95740

Merged
merged 1 commit into from Oct 30, 2020
Merged

Move cloud-controller-manager to staging k8s.io/cloud-provider #95740

merged 1 commit into from Oct 30, 2020

Conversation

cici37
Copy link
Contributor

@cici37 cici37 commented Oct 20, 2020

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This PR is to move code under cmd/cloud-controller-manager/app to k8s.io/cloud-provider.
Will close the issue kubernetes/cloud-provider#29

Which issue(s) this PR fixes:

Fixes kubernetes/cloud-provider#29

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. 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/dependency Issues or PRs related to dependency changes area/provider/gcp Issues or PRs related to gcp provider area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 20, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 20, 2020

PR merge currently is blocked by #91474.
Will remove WIP after the above PR merged.

@cici37
Copy link
Contributor Author

cici37 commented Oct 20, 2020

/sig cloud-provider

@cici37 cici37 force-pushed the moveCCM branch 2 times, most recently from 395cf58 to 05e3694 Compare October 20, 2020 22:05
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Oct 20, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 23, 2020

/test pull-kubernetes-conformance-kind-ipv6-parallel

@cheftako
Copy link
Member

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Oct 27, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 28, 2020

/test pull-kubernetes-conformance-kind-ipv6-parallel

@cici37
Copy link
Contributor Author

cici37 commented Oct 28, 2020

Hi @cheftako @andrewsykim , would you please review this PR when have time? Thank you 🙏

@cheftako
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2020
@sttts
Copy link
Contributor

sttts commented Oct 29, 2020

/approve

@dims
Copy link
Member

dims commented Oct 29, 2020

/assign @liggitt

@cheftako
Copy link
Member

/approve

liggitt
liggitt reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

one comment on the import restrictions, and needs a rebase

staging/src/k8s.io/cloud-provider/app/config/config.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package app
package config
Copy link
Member

Choose a reason for hiding this comment

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

this was weird before... thanks for fixing this

cmd/cloud-controller-manager/.import-restrictions Show resolved Hide resolved
andrewsykim
andrewsykim reviewed Oct 30, 2020
View reviewed changes
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

All my comments are unblocking, thanks @cici37 !

test/integration/serving/serving_test.go Outdated
@@ -32,7 +32,7 @@ import (
"k8s.io/apiserver/pkg/server/options"
"k8s.io/cloud-provider"
"k8s.io/cloud-provider/fake"
cloudctrlmgrtesting "k8s.io/kubernetes/cmd/cloud-controller-manager/app/testing"
cloudctrlmgrtesting "k8s.io/cloud-provider/testing"
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to keep this in under app based on it's previous location (i.e. k8s.io/cloud-provider/app/testing)?

staging/src/k8s.io/cloud-provider/config/scheme/scheme.go Outdated
@@ -20,8 +20,8 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/cloud-provider/app/apis/config"
"k8s.io/cloud-provider/app/apis/config/v1alpha1"
"k8s.io/cloud-provider/config"
Copy link
Member

Choose a reason for hiding this comment

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

This package should probably be called "install" instead?

Copy link
Member

Choose a reason for hiding this comment

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

Was referring to k8s.io/cloud-provider/config/scheme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just going to ask^^. Thanks for the further explain :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I might also change the func name to install instead of AddToSchema

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 30, 2020

Updated the pr since last review to:

  1. Added back .import-restrictions to cmd/cloud-controller-manager to ensure no new k8s.io/kubernetes deps get added.
  2. Moved k8s.io/cloud-provider/testing to k8s.io/cloud-provider/app/testing
  3. Changed package name from k8s.io/cloud-provider/config/scheme to k8s.io/cloud-provider/config/install and also changed the func name inside to install.

@liggitt Would you please have a look again when you have time? Thank you 🙏

@cici37
Copy link
Contributor Author

cici37 commented Oct 30, 2020

/test pull-kubernetes-node-e2e

@liggitt
Copy link
Member

liggitt commented Oct 30, 2020

/approve

@cheftako has lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, cici37, liggitt, sttts

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 Oct 30, 2020
@cheftako
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 30, 2020

/retest

@cici37
Copy link
Contributor Author

cici37 commented Oct 30, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@cici37
Copy link
Contributor Author

cici37 commented Oct 30, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 4b65f70 into kubernetes:master Oct 30, 2020
@cici37 cici37 deleted the moveCCM branch October 30, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheftako cheftako cheftako left review comments

@liggitt liggitt liggitt left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@cheftako cheftako

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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.

Move cmd/cloud-controller-manager and its controllers to k8s.io/cloud-provider (staging)
9 participants
@cici37 @fedebongio @fejta-bot @cheftako @sttts @dims @liggitt @k8s-ci-robot @andrewsykim