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
Add Apply and ApplyStatus methods to dynamic ResourceInterface #109443
Conversation
I think the only question I have is around whether I did the empty managed fields check correctly (and gave an appropriate error message). My notes from when we discussed this months ago say that we need to fail if the metadata has any managed fields in it in order to prevent people from passing in a fully-specified unstructured object they got via a read (e.g. /assign @lavalamp as the original requestor of this feature many moons ago |
d856de9
to
581ac7f
Compare
} | ||
managedFields := accessor.GetManagedFields() | ||
if len(managedFields) > 0 { | ||
return nil, fmt.Errorf(`cannot apply an object with managed fields already set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully the server does this check? (It's OK if the client does it too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it does :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! |
I'd actually like to backport it but I'm not sure we can claim it's a bug... |
/milestone v1.25 |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kevindelgado, lavalamp 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 |
/triage accepted |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds the
Apply
andApplyStatus
methods to the dynamicResourceInterface
to mirror the Apply and ApplyStatus methods already available on the typed clients, so that users do not have to manually construct Apply requests as Patch requests.Which issue(s) this PR fixes:
Fixes #105082
Special notes for your reviewer:
For testing I changed all tests in
test/integration/apiserver/apply/...
that calldynamicClient#Patch
with the apply patch type to usedynamicClient#Apply
instead.This seems like a sufficient way to test it. I didn’t add any testing in the dynamic client fake tests because there aren’t any existing Patch tests that use
types.ApplyPatchType
(and that’s maybe outside the scope of this PR)Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: