✨ Add DryRunClient wrapper #839
Conversation
k8s-ci-robot
added
cncf-cla: yes
alvaroaleman
force-pushed
the
client-dry-run
branch
from
c6fce28
to
535b63c
Compare
| kscheme "k8s.io/client-go/kubernetes/scheme" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/client" |
There was a problem hiding this comment.
Only curious because it looks intentional and not the kind of thing I would expect gofmt to do. Why this change?
There was a problem hiding this comment.
Because we usually put imports from our own repo in a dedicated group, rather than mixing them with other imports in a common group
alvaroaleman
force-pushed
the
client-dry-run
branch
from
535b63c
to
9fc31ad
Compare
alvaroaleman
force-pushed
the
client-dry-run
branch
from
9fc31ad
to
6981b0a
Compare
|
/assign @DirectXMan12 |
pkg/client/client.go
Outdated
| @@ -37,6 +37,9 @@ type Options struct { | |||
|
|
|||
| // Mapper, if provided, will be used to map GroupVersionKinds to Resources | |||
| Mapper meta.RESTMapper | |||
|
|
|||
| // DryRun specifies whether dry run mode should be enforced | |||
| DryRun bool | |||
There was a problem hiding this comment.
What's the main usecase for having this as an option here vs just letting people call NewDryRunClient? Just want to avoid too many options here, esp since this isn't strictly related to creating the client
There was a problem hiding this comment.
Good point, its not really necessary there, I removed it.
I would like to keep it in the manager though, because the defaultNewClient is not exported and replacing the NewClient func is not very intuitive and an option ppl that are not deeply familiar with c-r will most likely not become aware of. WDYT?
There was a problem hiding this comment.
Not super excited about growing manager's Options.
One alternative we can try here is that:
Making defaultNewClient exportable, but under pkg/manager/internal or pkg/internal/manager or some place similar.
Then you can have a helper method that satisfy NewClientFunc
func NewDryRunClientFn(cache cache.Cache, config *rest.Config, options client.Options) (client.Client, error) {
cl, err := managerinternal.DefaultNewClient(cache, config, options)
return &dryRunClient{cl}, err
}
User can pass this method to manager Options.
@DirectXMan12 Thoughts?
There was a problem hiding this comment.
@mengqiy the reason I dislike that is that its pretty hard to discover that these kind of build-in opts exists
alvaroaleman
force-pushed
the
client-dry-run
branch
from
6981b0a
to
e5ce18f
Compare
|
/assign @DirectXMan12 |
|
@DirectXMan12 any chance you can have a look? |
|
hey, sorry for the lag /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, DirectXMan12 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 |
k8s-ci-robot
added
the
approved
alvaroaleman
force-pushed
the
client-dry-run
branch
from
e5ce18f
to
626063b
Compare
k8s-ci-robot
removed
the
lgtm
|
@DirectXMan12 could you give a new lgtm as I had to rebase? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
FWIW, I'd love to eventually figure out how to make it easy to sub in the dryrun client instead of needing an option on manager, but I don't really think I want to block on that now |
This PR adds a DryRunClient wrapper that enforced DryRun on all mutating operations and plugs that into the client and manager constructors. This is useful to check what a controller does in a production environment without actually doing it.
Fixes #831