-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
🐛 fix: webhook testEnv #4449
base: master
Are you sure you want to change the base?
🐛 fix: webhook testEnv #4449
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mateusoliveira43 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 |
Hi @mateusoliveira43. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
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 one will require a safer approach and a deeper analysis to ensure that these changes are truly necessary and/or the best approach for the default scaffolds.
Currently, we have envtests
implemented and everything is functioning correctly, as shown here:
cronjob_webhook_test.go
I don’t believe that webhook tests are heavily used by users at the moment, but that doesn’t seem to be the case for controllers. Additionally, we already have mocks with tests that are functioning correctly for controllers.
Also, we’ve never added the CRDInstallOptions
option—it doesn’t seem to be required.
Could we raise an issue explaining why this change is necessary?
Could you provide more detailed information in the description here?
It would be helpful if you could let us know:
- What you tried to do and what didn’t work as expected.
- What challenges you faced and what you expected to see instead that led you to make each change.
(For example, moving theAddSchemes
, removing the import, and adding the optional spec forenvtest
.)
/hold until we have those details and ensure that each change is required/and or is the best approach for the default scaffolds.
Btw, thanks a lot for looking on that.
@camilamacedo86 will undo change to
Will add more info to PR description |
0c36daf
to
d540c85
Compare
// cfg is defined in this file globally. | ||
cfg, err = testEnv.Start() | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(cfg).NotTo(BeNil()) | ||
|
||
/* |
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.
I think we can reduce the scope of this one
We can have a PR to add the schemas before start the envtest
Then, we can have a PR that propose we remove apimachineryruntime and use scheme.Scheme and explain why we want to do that
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.
done #4466
will open next one after that one merges
Can you rebase it with the master and ensure that you have what was changed and why in the description? |
Signed-off-by: Mateus Oliveira <[email protected]>
d540c85
to
f645d9b
Compare
@camilamacedo86 updated |
Enable conversion webhooks for webhook
suite_test.go
envtest
by usingk8s.io/client-go/kubernetes/scheme
instead of creating new scheme (this also already done in controllersuite_test.go
)