Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Align FederatedTypeConfig API with K8s API conventions #885

Merged
merged 3 commits into from
May 16, 2019
Merged

Align FederatedTypeConfig API with K8s API conventions #885

merged 3 commits into from
May 16, 2019

Conversation

font
Copy link
Contributor

@font font commented May 14, 2019

Partial fix for #879.

@k8s-ci-robot k8s-ci-robot added 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. labels May 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 14, 2019
@font font mentioned this pull request May 14, 2019
Closed
6 tasks
marun
marun suggested changes May 14, 2019
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Please add fixup commits where changes are required with the expectation of squashing before merge.

Once this PR is ready for merge, I'd also like to see helm chart sync and code generation changes separated into their own commits.

config/enabletypedirectives/services.yaml Show resolved Hide resolved
pkg/apis/core/typeconfig/util.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
font
font commented May 14, 2019
View reviewed changes
Copy link
Contributor Author

@font font left a comment

Choose a reason for hiding this comment

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

@marun I resisted making most changes beyond just the API structs in order to keep the size of the PR down and to focus on what's most important for beta. I prefer to make the API interface and method changes in another PR. WDYT?

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Outdated Show resolved Hide resolved
config/enabletypedirectives/services.yaml Show resolved Hide resolved
pkg/apis/core/typeconfig/util.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Outdated Show resolved Hide resolved
@font
Copy link
Contributor Author

font commented May 14, 2019

@marun Updated based on our offline conversation and squashed commits as requested. PTAL.

marun
marun approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go
@@ -122,6 +136,7 @@ type FederatedTypeConfig struct {
// FederatedTypeConfigList contains a list of FederatedTypeConfig
type FederatedTypeConfigList struct {
metav1.TypeMeta `json:",inline"`
// +optional
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) ugh, why is this required again? It seems particularly odious to me without an accompanying comment. Is omitempty not enough of an indication that it's optional, or would there be a case where the +optional and omitempty would diverge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is highly recommended per https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required in the event the backwards compatibility guarantee of omitempty alone is not enough.

pkg/apis/core/v1alpha1/federatedtypeconfig_types.go Outdated Show resolved Hide resolved
pkg/controller/federatedtypeconfig/controller.go
typeConfig.Status.StatusController = corev1a1.ControllerStatusNotRunning

if typeConfig.Status.StatusController == nil {
typeConfig.Status.StatusController = new(corev1a1.ControllerStatus)
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Ugh, I think StatusController and PropagationController are terrible names for fields indicating whether a controller is running or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will probably be another opportunity to update this post API review, but if we want to change these now is also a good time.

@marun
Copy link
Contributor

marun commented May 14, 2019

Please rebase

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: font

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

@font
Copy link
Contributor Author

font commented May 14, 2019

@marun Updated and rebased.

@marun
Copy link
Contributor

marun commented May 15, 2019

LGTM

@irfanurrehman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit c4c8e6a into kubernetes-retired:master May 16, 2019
@font font deleted the ftc-api branch May 22, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marun marun marun approved these changes

@pmorie pmorie Awaiting requested review from pmorie

@shashidharatd shashidharatd Awaiting requested review from shashidharatd

@irfanurrehman irfanurrehman Awaiting requested review from irfanurrehman

@xunpan xunpan Awaiting requested review from xunpan

@danehans danehans Awaiting requested review from danehans

@quinton-hoole quinton-hoole Awaiting requested review from quinton-hoole

Assignees

@irfanurrehman irfanurrehman

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 Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@font @marun @k8s-ci-robot @irfanurrehman @pmorie