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

Remove dependency pkg/features from CCM #92201

Merged
merged 1 commit into from Oct 6, 2020
Merged

Remove dependency pkg/features from CCM #92201

merged 1 commit into from Oct 6, 2020

Conversation

cici37
Copy link
Contributor

@cici37 cici37 commented Jun 16, 2020 •
edited

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug

/kind cleanup

/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

It is part of the effort to make CCM easier to consume(kubernetes/cloud-provider#29).

Which issue(s) this PR fixes:

Fixes #

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 kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. 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-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 16, 2020
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 16, 2020
@cici37
Copy link
Contributor Author

cici37 commented Jun 16, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 16, 2020
@cici37 cici37 changed the title Remove dependency pkg/features from CCM [WIP]Remove dependency pkg/features from CCM Jun 16, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2020
@cici37
Copy link
Contributor Author

cici37 commented Jun 17, 2020

/retest

1 similar comment
@cici37
Copy link
Contributor Author

cici37 commented Jun 17, 2020

/retest

@cici37 cici37 changed the title [WIP]Remove dependency pkg/features from CCM Remove dependency pkg/features from CCM Jun 17, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2020
@fedebongio
Copy link
Contributor

/assign @cheftako @jiahuif

@cici37
Copy link
Contributor Author

cici37 commented Jun 23, 2020

/retest

@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 Sep 30, 2020
@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 Sep 30, 2020
@cici37
Copy link
Contributor Author

cici37 commented Sep 30, 2020 •
edited

@deads2k I have separated the setup func with init func and also moved the file to k8s.io/controller-manager. Would you please have a look when you have time? Thank you 😊

andrewsykim
andrewsykim reviewed Sep 30, 2020
View reviewed changes
cmd/cloud-controller-manager/.import-restrictions Outdated
@@ -9,6 +9,7 @@ rules:
- k8s.io/kubernetes/cmd/cloud-controller-manager/app/options
- k8s.io/kubernetes/cmd/controller-manager/app
- k8s.io/kubernetes/cmd/controller-manager/app/options
- k8s.io/kubernetes/cmd/controller-manager/pkg/features
Copy link
Member

Choose a reason for hiding this comment

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

Should this be k8s.io/controller-manager/pkg/features?

Copy link
Member

Choose a reason for hiding this comment

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

Or removed I guess since this list is only for k8s.io/kubernetes imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to remove this import ^^

staging/src/k8s.io/controller-manager/pkg/features/featureconst/kube_features.go Outdated
limitations under the License.
*/

package featureconst
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that a new package featureconst is needed to store the feature constants, just k8s.io/controller-manager/pkg/features/kube_features would be enough I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that the init func will be called whenever people import k8s.io/controller-manager/pkg/feature and we would like to separate it for the use case like allow people to depend on teh consts without gettng auto-registration(suggested by @deads2k ).

Copy link
Member

Choose a reason for hiding this comment

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

I believe Davids idea was to allow people to pick up the constants and be able to choose whether to pick up the auto-registration. The idea is to move away from the auto-registration magic. I wonder if it would make more sense to split out the init (auto-registration) into an autoregister package, so that the behavior there is a bit more explicit?

Copy link
Member

Choose a reason for hiding this comment

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

Could we do this by having a single package k8s.io/controller-manager/pkg/feature and remove init()? That way you can consume constants, but registering gates would requier an explicit call to SetupCurrentKubernetesSpecificFeatureGates. Or was the separate package added because we want to keep init() for registration?

Copy link
Member

Choose a reason for hiding this comment

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

If we go the auto-registration route, maybe k8s.io/controller-manager/pkg/features/register might be cleaner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. Updated to use k8s.io/controller-manager/pkg/features/register. Thank you 😊

@cici37 cici37 force-pushed the features branch 3 times, most recently from 83297fb to 5f07d35 Compare October 1, 2020 22:21
@cici37
Copy link
Contributor Author

cici37 commented Oct 2, 2020

/test pull-publishing-bot-validate

@cici37
Copy link
Contributor Author

cici37 commented Oct 5, 2020

/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-integration

@cici37
Copy link
Contributor Author

cici37 commented Oct 5, 2020

/test pull-kubernetes-bazel-build

@deads2k
Copy link
Contributor

deads2k commented Oct 6, 2020

/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 Oct 6, 2020
@lavalamp
Copy link
Member

lavalamp commented Oct 6, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, cici37, deads2k, lavalamp

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 6, 2020
@cici37
Copy link
Contributor Author

cici37 commented Oct 6, 2020

/test pull-kubernetes-bazel-test

@cici37
Copy link
Contributor Author

cici37 commented Oct 6, 2020

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

@k8s-ci-robot k8s-ci-robot merged commit a1441b2 into kubernetes:master Oct 6, 2020
@cici37 cici37 deleted the features branch October 6, 2020 22:59
@cheftako cheftako mentioned this pull request Feb 25, 2021
Merged
@liggitt liggitt mentioned this pull request Mar 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@cheftako cheftako cheftako left review comments

@liggitt liggitt liggitt left review comments

@deads2k deads2k deads2k left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@thockin thockin Awaiting requested review from thockin

Assignees

@cheftako cheftako

@liggitt liggitt

@jiahuif jiahuif

@deads2k deads2k

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/dependency Issues or PRs related to dependency changes area/release-eng Issues or PRs related to the Release Engineering subproject 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet