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

🏃Rename EnsureOwnerReference to SetOwnerReference for consistency #844

Merged
merged 1 commit into from
Mar 10, 2020
Merged

🏃Rename EnsureOwnerReference to SetOwnerReference for consistency #844

merged 1 commit into from
Mar 10, 2020

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Mar 9, 2020

To be consistent with SetControllerReference, rename EnsureOwnerReference to SetOwnerReference.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 9, 2020
@vincepri
Copy link
Member Author

vincepri commented Mar 9, 2020

Given that we haven't released v0.5.1 yet, we can consider this as non-breaking in the current cycle

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 Mar 9, 2020
@vincepri vincepri added this to the v0.5.x milestone Mar 9, 2020
@alvaroaleman
Copy link
Member

Why do we actually have these two, does the controller boolean make any difference at all?

@vincepri
Copy link
Member Author

vincepri commented Mar 9, 2020 •
edited
Loading

In some cases you might want multiple OwnerReferences, only one can be set (when using these or metav1 utilities, not actually sure if api server throws an error). Multiple OwnerRefs can be useful when you want to create a tree of dependencies between objects

djzager
djzager suggested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@djzager djzager left a comment

Choose a reason for hiding this comment

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

In some cases you might want multiple OwnerReferences, only one can be set (when using these or metav1 utilities, not actually sure if api server throws an error). Multiple OwnerRefs can be useful when you want to create a tree of dependencies between objects

Now seems like a good time to go ahead and document this. I know from experience that when I have scoured godoc that an explanation why I might want to use SetOwnerReference would be helpful.

pkg/controller/controllerutil/controllerutil.go Show resolved Hide resolved
@vincepri
Rename EnsureOwnerReference to SetOwnerReference for consistency
ca1f091 
Signed-off-by: Vince Prignano <vincepri@vmware.com>
alvaroaleman
alvaroaleman approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
so @djzager can check on the comment

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2020
@DirectXMan12
Copy link
Contributor

/hold cancel

as per above approval

@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 Mar 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5747307 into kubernetes-sigs:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djzager djzager djzager approved these changes

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@gerred gerred Awaiting requested review from gerred

Assignees

@alvaroaleman alvaroaleman

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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5.x
Development

Successfully merging this pull request may close these issues.
5 participants
@vincepri @k8s-ci-robot @alvaroaleman @DirectXMan12 @djzager