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

🐛 EnqueueRequestForOwner correctly enqueue cluster-scoped owner #274

Merged
merged 1 commit into from
Jan 16, 2019
Merged

🐛 EnqueueRequestForOwner correctly enqueue cluster-scoped owner #274

merged 1 commit into from
Jan 16, 2019

Conversation

shawn-hurley
Copy link

Determines if the owner object is cluster scoped, and if it is will not use the object's namespace in the reconcile request.

To complete this, it adds dependency injection for the managers RESTMapper.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 3, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 3, 2019
@pwittrock
Copy link
Contributor

Cool. IIUC - if both the owned and owning objects are namespaced things work correctly. If both are not-namespaced things work correctly. This addresses the case where a non-namespaced object owns a namespaced object. I am curious, are there usecases for this pattern you can share?

DirectXMan12
DirectXMan12 suggested changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

comments inline, generally looks good

pkg/handler/enqueue_owner.go Outdated Show resolved Hide resolved
pkg/handler/enqueue_owner.go Outdated Show resolved Hide resolved
pkg/handler/eventhandler_suite_test.go
testenv = &envtest.Environment{}
var err error
cfg, err = testenv.Start()
Expect(err).NotTo(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just extract this out onto a helper on testenv at some point? I want to be able to call BeforeSuite(testenv.Environment{}.BeforeSuite) or something similar.

Copy link
Author

Choose a reason for hiding this comment

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

Should I create an issue to track this or is it already tracked some other place?

@droot
Copy link
Contributor

droot commented Jan 4, 2019

Changes look good. I have the same question as @pwittrock are there any use-cases for non-namespaced object owns a namespaced object ?

@DirectXMan12 DirectXMan12 mentioned this pull request Jan 4, 2019
Closed
@shawn-hurley
Copy link
Author

@pwittrock @droot #228 is the bug that I was attempting to fix.

EnqueueRequestForOwner doesn't support this relationship because it uses the namespace of the owned object as the namespace of the owner. We ran into this in knative/eventing#649.

This is the part that you might be interested in for they use case.

@droot
Copy link
Contributor

droot commented Jan 11, 2019

The change looks good to me. Will leave it to @DirectXMan12 to lgtm it.

@DirectXMan12
Copy link
Contributor

/lgtm
/approve
looks like there's a conflict with another pr. please rebase, and we'll get this merged.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2019
@DirectXMan12
Copy link
Contributor

/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 Jan 16, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, shawn-hurley

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 merged commit 752e0b5 into kubernetes-sigs:master Jan 16, 2019
@fabxc
Copy link

fabxc commented Feb 20, 2019

I ran into this issue and upgraded to a version that includes this commit. It doesn't seem to fix the issue for me.

@DirectXMan12
Copy link
Contributor

DirectXMan12 commented Feb 26, 2019 •
edited
Loading

@fabxc can you post a reproducer or test or something to quickly verify?

@prometherion prometherion mentioned this pull request Apr 9, 2019
Closed
This was referenced Aug 19, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 approved these changes

@droot droot Awaiting requested review from droot

Assignees

@DirectXMan12 DirectXMan12

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

None yet
6 participants
@shawn-hurley @pwittrock @droot @DirectXMan12 @k8s-ci-robot @fabxc