:sparkles Add env support for setting ErrorIfCRDPathMissing during testing #511
Conversation
|
Welcome @connor1989! |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: connor1989 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
connor1989
changed the title
|
/assign @pwittrock |
|
@connor1989: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -58,10 +58,28 @@ var _ = Describe("Test", func() { | |||
| }) | |||
|
|
|||
| Describe("InstallCRDs", func() { | |||
|
|
|||
| It("should set ErrorIfCRDPathMissing through env settings", func(done Done) { | |||
There was a problem hiding this comment.
it's unclear to me what this is testing, since it doesn't seem to be testing that the setting gets automatically propogated from Environment to InstallCRDs
There was a problem hiding this comment.
Yes, it only validates the new option. This was to keep the other tests unchanged here.
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/envtest/envtest_test.go#L155
My other thought was to make this change directly in BeforeSuite and update all the tests accordingly.
var _ = BeforeSuite(func(done Done) {
logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))
env = &Environment{
++ ErrorIfCRDPathMissing: true,
}
_, err := env.Start()
Any opposition to this?
| It("should install the CRDs into the cluster", func(done Done) { | ||
|
|
||
| crds, err = InstallCRDs(env.Config, CRDInstallOptions{ | ||
| Paths: []string{filepath.Join(".", "testdata")}, | ||
| Paths: []string{filepath.Join(".", "testdata")}, | ||
| ErrorIfPathMissing: env.ErrorIfCRDPathMissing, |
There was a problem hiding this comment.
@DirectXMan12 to your first point, although the change is minimal, I did make use of the new option here
There was a problem hiding this comment.
right, but it's not plumbed through to any non-test logic, afaict
| It("should install the CRDs into the cluster", func(done Done) { | ||
|
|
||
| crds, err = InstallCRDs(env.Config, CRDInstallOptions{ | ||
| Paths: []string{filepath.Join(".", "testdata")}, | ||
| Paths: []string{filepath.Join(".", "testdata")}, | ||
| ErrorIfPathMissing: env.ErrorIfCRDPathMissing, |
There was a problem hiding this comment.
right, but it's not plumbed through to any non-test logic, afaict
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/stale
|
@connor1989: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
A simpler interface for CRDInstallOptions.ErrorIfPathMissing. Adds ability to control, through the env, how errors are propagated when InstallCRDs is invoked with an invalid CRD path.
Closes #481