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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources #5383

Merged
merged 1 commit into from Oct 25, 2021
Merged

馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources #5383

merged 1 commit into from Oct 25, 2021

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Oct 5, 2021 •
edited

Signed-off-by: Stefan B眉ringer buringerst@vmware.com

What this PR does / why we need it:
Just saw in @randomvariable's code walkthrough that we're not using the namespace for the List call.

WIP: pending PR in controller-runtime to fix the unit test.

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 5, 2021
sbueringer
sbueringer commented Oct 5, 2021
View reviewed changes
util/util.go Outdated Show resolved Hide resolved
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch from 22b4a6d to 52408fd Compare October 5, 2021 11:41
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 5, 2021
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch from 52408fd to 2bb7203 Compare October 5, 2021 11:44
@sbueringer sbueringer changed the title 馃尡 ClusterToObjectsMapper: use namespace in client.List 馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources Oct 5, 2021
sbueringer
sbueringer commented Oct 5, 2021
View reviewed changes
util/util.go Show resolved Hide resolved
@randomvariable
Copy link
Member

Just saw in @randomvariable's code walkthrough that we're not using the namespace for the List call.

Thought that was weird.

vincepri
vincepri reviewed Oct 5, 2021
View reviewed changes
util/util.go Show resolved Hide resolved
vincepri
vincepri reviewed Oct 5, 2021
View reviewed changes
util/util.go Outdated Show resolved Hide resolved
util/util.go Show resolved Hide resolved
@sbueringer sbueringer mentioned this pull request Oct 5, 2021
Merged
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch from 2bb7203 to 9d132ea Compare October 6, 2021 07:51
@sbueringer sbueringer changed the title 馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources [WIP] 馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources Oct 6, 2021
@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 Oct 6, 2021
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch 2 times, most recently from 22bbc07 to c74e650 Compare October 7, 2021 16:36
@sbueringer sbueringer changed the title [WIP] 馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources 馃尡 ClusterToObjectsMapper: use namespace in client.List for namespaced resources Oct 7, 2021
@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 Oct 7, 2021
@sbueringer
Copy link
Member Author

fake client PR in CR is merged: kubernetes-sigs/controller-runtime#1686

Code should be ready for review

/hold
until the next CR v0.10.x release

@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 Oct 7, 2021
@sbueringer
Copy link
Member Author

@vincepri PR should be ready now

@vincepri
Copy link
Member

vincepri commented Oct 18, 2021 •
edited

We shouldn't use a commit of Controller Runtime, let's wait for a release before merging

@sbueringer
Copy link
Member Author

We shouldn't use a commit of Controller Runtime, let's wait for a release before merging

Agree, that's why I added the hold in #5383 (comment)

I only meant it's ready for review, not for merge :)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2021
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch from c74e650 to a33f808 Compare October 25, 2021 07:26
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021
@vincepri
Copy link
Member

Given that we're targeting 1.1 for this change, let's merge it for now and make sure CR is updated later

@sbueringer
ClusterToObjectsMapper: use namespace in client.List
556258b 
Signed-off-by: Stefan B眉ringer buringerst@vmware.com
@sbueringer sbueringer force-pushed the pr-namespace-clustertoobjectsmapper-list branch from a33f808 to 556258b Compare October 25, 2021 16:50
@sbueringer
Copy link
Member Author

Given that we're targeting 1.1 for this change, let's merge it for now and make sure CR is updated later

Sounds good. I rebased onto the latest commit of the release-0.10 branch of CR. Let me know if I should use something else.

vincepri
vincepri approved these changes Oct 25, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
/hold cancel

@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 Oct 25, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2021
@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 Oct 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 30dbae0 into kubernetes-sigs:main Oct 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

Assignees

@vincepri vincepri

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@sbueringer @randomvariable @vincepri @k8s-ci-robot