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 defaulting from shared ComponentConfig types #67207

Merged
merged 1 commit into from
Aug 10, 2018
Merged

Remove defaulting from shared ComponentConfig types #67207

merged 1 commit into from
Aug 10, 2018

Conversation

luxas
Copy link
Member

@luxas luxas commented Aug 9, 2018

What this PR does / why we need it:

As @deads2k commented in kubernetes/community#2354, we should not register defaults for the shared componentconfig types as it gets very hard for consumer to opt-out of the default defaulting funcs. Instead, the package provides a DefaultFoo function the consuming API group can call if it wants to as an opt-in in SetDefaults_Bar (where Bar wraps Foo as a field)

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

Special notes for your reviewer:

Release note:

NONE

/assign @sttts @liggitt @deads2k

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 9, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 9, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018
@luxas luxas mentioned this pull request Aug 9, 2018
Merged
4 tasks
deads2k
deads2k reviewed Aug 9, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/defaults.go Outdated
}

func SetDefaults_ClientConnectionConfiguration(obj *ClientConnectionConfiguration) {
func DefaultClientConnectionConfiguration(obj *ClientConnectionConfiguration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

RecommendedDefaultClientConnectionConfiguration. godoc indicating that it is subject to change as our recommendations change (same as the options structs we have today in apiserver) and an explanation that providing this method is a courtesy if you find that wish to use it in your config. It is intentionallly not registered in the scheme so that callers have flexibility to choose their own defaults. Conversion relies on defaulting, so if we did not do this, our conversion may accidentally rely on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

added godoc comment to both types

liggitt
liggitt reviewed Aug 9, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1/register.go Outdated
var (
// localSchemeBuilder and AddToScheme will stay in k8s.io/kubernetes.
SchemeBuilder runtime.SchemeBuilder
localSchemeBuilder = &SchemeBuilder
Copy link
Member

Choose a reason for hiding this comment

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

zz_generated.conversion.go will be unhappy with this being gone, I expect

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah of course, was a bit too quick in removing, readded

liggitt
liggitt reviewed Aug 9, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/apis/config/v1alpha1/register.go Outdated
var (
// localSchemeBuilder and AddToScheme will stay in k8s.io/kubernetes.
SchemeBuilder runtime.SchemeBuilder
localSchemeBuilder = &SchemeBuilder
Copy link
Member

Choose a reason for hiding this comment

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

same here, conversion needs this, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

readded

@luxas luxas force-pushed the remove_shared_config_defaulting branch from 5ed6ed0 to a4103ae Compare August 9, 2018 19:42
This was referenced Aug 9, 2018
Merged
Closed
@luxas luxas force-pushed the remove_shared_config_defaulting branch from a4103ae to 1b23465 Compare August 9, 2018 20:34
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2018
@stewart-yu
Copy link
Contributor

/test pull-kubernetes-e2e-kops-aws

@neolit123
Copy link
Member

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 10, 2018
@stewart-yu stewart-yu mentioned this pull request Aug 10, 2018
Closed
15 tasks
@stewart-yu
Copy link
Contributor

LGTM

@deads2k
Copy link
Contributor

deads2k commented Aug 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, luxas

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 Aug 10, 2018
@luxas
Copy link
Member Author

luxas commented Aug 10, 2018

/test pull-kubernetes-integration

@luxas
Copy link
Member Author

luxas commented Aug 10, 2018

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 66602, 67178, 67207, 67125, 66332). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 94a754c into kubernetes:master Aug 10, 2018
k8s-github-robot pushed a commit that referenced this pull request Aug 15, 2018
Merge pull request #66722 from luxas/reference_new_generic_structs
698eb76 
Automatic merge from submit-queue (batch tested with PRs 67071, 66906, 66722, 67276, 67039). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove references to the structs that have moved to their own packages

**What this PR does / why we need it**:
Follows-up #66058 and  #66059 to remove the structs that now aren't needed in `pkg/apis/componentconfig`

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
xref kubernetes/community#2354

**Special notes for your reviewer**:

This PR depends on:
 - [x] #67090
 - [x] #67149
 - [x] #67159
 - [x] #67207

**Only review commit 'Remove references to the structs that have moved to their own packages' please**

**Release note**:

```release-note
NONE
```
/kind cleanup
/assign @sttts @thockin @jbeda @liggitt
@fedebongio
Copy link
Contributor

/assign @cheftako

k8s-github-robot pushed a commit that referenced this pull request Sep 2, 2018
Merge pull request #66993 from luxas/cleanup_componentconfig_types
b8e97d3 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

Refactor the kube-controller-manager ComponentConfig structs

**What this PR does / why we need it**:

This PR cleans up the kube-controller-manager structs in the componentconfig package and fixes various structural issues in the current code, in order to make it possible to later move the code out to external API groups (as a starting point `GenericControllerManagerConfiguration` to `k8s.io/controller-manager`).

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
ref: kubernetes/community#2354

This PR depends on:
 - [x] #67149
 - [x] #67090
 - [x] #67159
 - [x] #67207
 - [x] #66722

**Special notes for your reviewer**:

Please only review the following commits:
 - **Refactor the k-c-m ComponentConfig structs to they can be moved out**
 - **Fixup cmd/kube-controller-manager code after struct changes.**

**Release note**:

```release-note
NONE
```
/assign @sttts @stewart-yu @liggitt @thockin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@deads2k deads2k deads2k left review comments

@mikedanese mikedanese Awaiting requested review from mikedanese

Assignees

@sttts sttts

@cheftako cheftako

@liggitt liggitt

@deads2k deads2k

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-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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@luxas @stewart-yu @neolit123 @deads2k @k8s-ci-robot @k8s-github-robot @fedebongio @liggitt @sttts @cheftako