Skip to content

⚠️ Fakeclient: Fix status handling#2259

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
alvaroaleman:fake-status
Apr 14, 2023
Merged

⚠️ Fakeclient: Fix status handling#2259
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
alvaroaleman:fake-status

Conversation

@alvaroaleman
Copy link
Copy Markdown
Member

@alvaroaleman alvaroaleman commented Apr 1, 2023 •
edited
Loading

This change fixes the status handling of the fake client by

  • Adding a new WithStatusSubresource option and pre-poluating it with
    all in-tree resources that have a status subresource
  • Making its Update and Patch not change the status of any such
    resource
  • Making its status client Update and Patch only change the status
    for any such resources

Since this was completely broken before in that both non-status and
status Update/Patch would always update everything and the status
resources get pre-populated, this is a breaking change. Any test that
relied on the previous behavior would pass incorrectly though, as it
didn't match what the Kubernetes API does.

Fixes #2230

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Apr 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 1, 2023
@k8s-ci-robot k8s-ci-robot requested review from FillZpp and vincepri April 1, 2023 03:13
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2023
@alvaroaleman alvaroaleman mentioned this pull request Apr 1, 2023
Closed
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 1, 2023
@alvaroaleman alvaroaleman force-pushed the fake-status branch 2 times, most recently from ea6cd8c to 927b57f Compare April 1, 2023 04:50
@alvaroaleman alvaroaleman changed the title ⚠️ Fakeclient: Drop status changes in Update/Patch if configured ⚠️ Fakeclient: Fix status handling Apr 10, 2023
@alvaroaleman
Copy link
Copy Markdown
Member Author

alvaroaleman commented Apr 10, 2023

/assign @FillZpp

@alvaroaleman alvaroaleman force-pushed the fake-status branch 2 times, most recently from f438bb8 to 388a58d Compare April 10, 2023 03:34
@FillZpp
Copy link
Copy Markdown
Contributor

FillZpp commented Apr 10, 2023

/retest

@FillZpp
Copy link
Copy Markdown
Contributor

FillZpp commented Apr 11, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2023
@alvaroaleman
Copy link
Copy Markdown
Member Author

alvaroaleman commented Apr 11, 2023

/hold
I realized there is one piece missing: We need to error if status is used on objects that do not have the status subresource

@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 Apr 11, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2023
@alvaroaleman
Copy link
Copy Markdown
Member Author

alvaroaleman commented Apr 11, 2023

I realized there is one piece missing: We need to error if status is used on objects that do not have the status subresource

Added that.

/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 Apr 11, 2023
@alvaroaleman
⚠️ Fakeclient: Fix status handling
0bff6fc 
This change fixes the status handling of the fake status client by

* Adding a new `WithStatusSubresource` option and pre-poluating it with
  all in-tree resources that have a status subresource
* Making its `Update` and `Patch` not change the status of any such
  resource
* Making its status client `Update` and `Patch` only change the status
  for any such resources
* Making it error if `Status().{Update,Patch}` is called on a resource
  that isn't configured to have the status subresource

Since this was completely broken before in that both non-status and
status Update/Patch would always update everything and the status
resources get pre-populated, this is a breaking change. Any test that
relied on the previous behavior would pass incorrectly though, as it
didn't match what the Kubernetes API does.
@sbueringer sbueringer self-requested a review April 12, 2023 14:20
@sbueringer sbueringer removed their assignment Apr 12, 2023
@vincepri vincepri added this to the v0.15.x milestone Apr 12, 2023
aleskandro added a commit to aleskandro/machine-api-provider-azure that referenced this pull request Jun 5, 2023
@aleskandro
Fix the fakeClient to include the relevant GVKs in the withStatusSubr…
08cffd7 
…esource set

kubernetes-sigs/controller-runtime#2259 fixed the status handling of the fake client by adding a new WithStatusSubresource method to include the GVKs in a mock set used to distinguish whether an object being updated through the *fakeSubResourceClient.Patch method should be considered or throw errNotFound.

Ref.: https://github.com/kubernetes-sigs/controller-runtime/pull/2259/files#diff-20ecedbf30721c01c33fb67d911da11c277e29990497a600d20cb0ec7215affdR387-R398
@tenzen-y tenzen-y mentioned this pull request Jun 20, 2023
Closed
@nicks nicks mentioned this pull request Jun 23, 2023
Closed
@RealAnna RealAnna mentioned this pull request Jul 12, 2023
Merged
@ary1992 ary1992 mentioned this pull request Jul 15, 2023
Closed
@RealAnna RealAnna mentioned this pull request Jul 17, 2023
Closed
7 tasks
@RainbowMango RainbowMango mentioned this pull request Jul 27, 2023
Merged
@Sunnatillo Sunnatillo mentioned this pull request Jul 27, 2023
Merged
@Huang-Wei Huang-Wei mentioned this pull request Jul 28, 2023
Merged
@Sunnatillo Sunnatillo mentioned this pull request Aug 1, 2023
Merged
@zhujian7 zhujian7 mentioned this pull request Aug 10, 2023
Merged
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 10, 2023
@lleshchi
Update unittests and scheme
9f8eb02 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 14, 2023
@lleshchi
Update unittests and scheme
d5694f7 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 15, 2023
@lleshchi
Update unittests and scheme
b77e5ef 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 15, 2023
@lleshchi
Update unittests and scheme
abfcfd6 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
@zishen zishen mentioned this pull request Aug 16, 2023
Merged
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 16, 2023
@lleshchi
Update unittests and scheme
5af4d4c 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
lleshchi added a commit to lleshchi/hive that referenced this pull request Aug 16, 2023
@lleshchi
Update unittests and scheme
26a0b81 
Rewrite unittests to accommodate changes in controller-runtime
v0.15.0 and create a hive specific scheme.

One such major change involves the behavior of the fake client
deletionTimestamp. The fake client will panic if initialized with an
object that has a DeletionTimestamp and no finalizer, and will
silently ignore a DeletionTimestamp on an object in a Create request,
matching the behavior of the kube-apiserver. Tests have been rewritten
to work with this new behavior.
kubernetes-sigs/controller-runtime#2316

This update also includes a requirement to register a fake client
WithStatusSubresource in order to successfully update the Status
subresource of an object. Due to this change, the helper function
NewFakeClientBuilder() is added to pkg/test/fake to allow for easier
setup of a fake client with the necessary sub-resources.
kubernetes-sigs/controller-runtime#2259

Lastly, a new singleton scheme has been introduced to the codebase.
The hive specific scheme is registered once, with all necessary
packages, and used throughout the codebase, removing the need to specify
unique schemes for each use case, and decreasing the overall amount of
imports.

Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>
@zishen zishen mentioned this pull request Aug 18, 2023
Closed
@Tamas-Biro1 Tamas-Biro1 mentioned this pull request Sep 4, 2023
Merged
@acumino acumino mentioned this pull request Sep 19, 2023
Closed
rhmdnd added a commit to ComplianceAsCode/compliance-operator that referenced this pull request Oct 25, 2023
@rhmdnd
Fix broken tests as a result of updating controller-runtime 0.16.2
976bc36 
The fake client we use in our unit tests was recently updated to be more
inline with what the actual kubernetes API does:

  kubernetes-sigs/controller-runtime#2259

As a result, some of our tests broken because we were lumping Status and
other updates into the same Update() request.

This commit refactors those tests so that Status updates are implemented
separately from the rest of the runtime objects. We need to do this
before we can upgrade controller-runtime to 0.16.3.
@rhmdnd rhmdnd mentioned this pull request Oct 25, 2023
Merged
1 task
rhmdnd added a commit to ComplianceAsCode/compliance-operator that referenced this pull request Oct 26, 2023
@rhmdnd
Fix broken tests as a result of updating controller-runtime 0.16.2
c7e9a30 
The fake client we use in our unit tests was recently updated to be more
inline with what the actual kubernetes API does:

  kubernetes-sigs/controller-runtime#2259

As a result, some of our tests broken because we were lumping Status and
other updates into the same Update() request.

This commit refactors those tests so that Status updates are implemented
separately from the rest of the runtime objects. We need to do this
before we can upgrade controller-runtime to 0.16.3.
yiannistri added a commit to weaveworks/weave-gitops-enterprise that referenced this pull request Nov 6, 2023
@yiannistri
fix: Add status subresource to fake client due to kubernetes-sigs/con…
c7da498 
…troller-runtime#2259
@shreyas-s-rao shreyas-s-rao mentioned this pull request Jan 7, 2024
Merged
12 tasks
@ranakan19 ranakan19 mentioned this pull request Mar 20, 2024
Merged
@ranakan19 ranakan19 mentioned this pull request Jul 9, 2024
Merged
@nickmisasi nickmisasi mentioned this pull request Nov 1, 2024
Merged
@erikgb erikgb mentioned this pull request Feb 1, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbueringer sbueringer sbueringer left review comments

@FillZpp FillZpp Awaiting requested review from FillZpp

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@FillZpp FillZpp

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

v0.15.x

Development

Successfully merging this pull request may close these issues.

Fake Client .Update updates both spec and status?

5 participants

@alvaroaleman @k8s-ci-robot @FillZpp @sbueringer @vincepri