-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Server-Side DryRun support to Client #338
Conversation
/assign @droot |
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.
you missed fakeClient, otherwise LGTM 👍
@DirectXMan12 I've updated the fake client to basically do a no-op when passed the |
yeah, I think that's sufficient. Looks like travis is complaining about 2 tests. |
I think this is because the K8s version being used in the tests is 1.10.1 which I guess silently ignores the DryRun option, am I ok to update this to a newer version?
controller-runtime/hack/check-everything.sh Lines 55 to 57 in 6649bdb
|
Yeah, go ahead and update to a newer version :-) |
1.13.1 is the latest, IIRC. I've turned on listing on that bucket, so you should be able to check yourself now. |
@DirectXMan12 I've bumped the K8s version and the tests are passing now |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, JoelSpeed 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 |
/hold |
technically this is breaking, since it changes an interface -- needed to update title /hold cancel |
This PR adds
CreateOptions
andUpdateOptions
to theClient
package to allow client users to set theDryRun
option when creating and updating resources using the client. This should allow users to see the result of all Admission controllers before persisting changes.I've attempted to match the patterns in
DeleteOptions
andListOptions
as much as possible but since the only valid value forDryRun
isall
(see here) I've not made that configurable, but it could be easily changed. I've also had to name the twoOptionFuncs
CreateDryRunAll
andUpdateDryRunAll
which I don't particularly like, has anyway any better ideas for names for these?