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.

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

Adapt to controller-runtime client changes #327

Merged
merged 1 commit into from Jan 4, 2023

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Jan 3, 2023

The controller-runtime client has new methods for interacting with sub resources. The fake client is updated to implement the new interface. Interacting with any sub resource, other than status, will trigger the reactor chain with no other pre-defined behavior. Custom reactors must be installed in order to observe/handle the request.

Resolves #325

The controller-runtime client has new methods for interacting with sub
resources. The fake client is updated to implement the new interface.
Interacting with any sub resource, other than status, will trigger the
reactor chain with no other pre-defined behavior. Custom reactors must
be installed in order to observe/handle the request.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Base: 61.55% // Head: 60.51% // Decreases project coverage by -1.04% ⚠️

Coverage data is based on head (7860f6e) compared to base (d9febe1).
Patch coverage: 20.83% of modified lines in pull request are covered.

❗ Current head 7860f6e differs from pull request most recent head 99f42ba. Consider uploading reports for the commit 99f42ba to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   61.55%   60.51%   -1.05%     
==========================================
  Files          17       17              
  Lines        2154     2193      +39     
==========================================
+ Hits         1326     1327       +1     
- Misses        751      789      +38     
  Partials       77       77              
Impacted Files Coverage Δ
testing/client.go 54.58% <7.31%> (-10.86%) ⬇️
testing/config.go 87.98% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you

[thought]: The ReconcilerTestCase has ExpectStatusUpdates and ExpectStatusPatches. Do you reckon that it should eventually move towards an API akin to ExpectSubResource[Updates, Patches]?

https://github.com/vmware-labs/reconciler-runtime/blob/main/testing/reconciler.go#L67-L70

@scothis
Copy link
Contributor Author

scothis commented Jan 4, 2023

LGTM! Thank you

[thought]: The ReconcilerTestCase has ExpectStatusUpdates and ExpectStatusPatches. Do you reckon that it should eventually move towards an API akin to ExpectSubResource[Updates, Patches]?

https://github.com/vmware-labs/reconciler-runtime/blob/main/testing/reconciler.go#L67-L70

I went down that rabbit hole initially. It's a breaking change that creates a worse experience for almost zero benefit. Using an ExpectSubResource* field wouldn't be able to accept client.Object, but would need to wrap that in some new struct that is aware of which sub resource is being called. Assertions are tricky because the sub resource can have different semantics. For example, the status sub resource, we ignore any field not under .status. Because of that the user already needs to provide a reactor to handle the request.

The new sub resource client doesn't feel fully baked. We need to support the interface in order to compile, but I'm not ready to build much that's more meaningful on top. We'll see how it goes.

@scothis scothis merged commit 3fe9f80 into vmware-labs:main Jan 4, 2023
@scothis scothis deleted the controller-runtime-0.14 branch January 4, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kubernetes 1.26.0
3 participants