-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
✨ (go/v4): improve the webhook tests by adding examples. Also, improve cronjob tutorial to clarify its usage and validate the changes #4130
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 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 |
@vsoch , @ashutosh887, @Sajiyah-Salat, @drewwells Could you please review this PR? |
4f44614
to
63bfcf5
Compare
ba09ad3
to
b4e13b8
Compare
In this PR we are improving the webhook tests by adding further info and examples for the users. Either we are implementing them further for the cronjob tutorial as an example and to help us to validate and spot issues on related areas when/if we need to change them
b4e13b8
to
6ed50ba
Compare
Assuming all the CI passes, this looks very helpful! Thanks @camilamacedo86 |
Context("When creating or updating CronJob under Validating Webhook", func() { | ||
It("Should deny creation if the name is too long", func() { | ||
obj.ObjectMeta.Name = "this-name-is-way-too-long-and-should-fail-validation-because-it-is-way-too-long" | ||
warnings, err := validator.ValidateCreate(ctx, obj) |
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 you can replace this with a single Error matcher:
Expect(validator.ValidateCreate(ctx, obj)).Error().To(MatchError(ContainSubstring("must be no more than 52 characters"))),
"Expected name validation to fail for a too-long name")
Gomega's error validator both matches the error, and that any other responses (warnings
) are zero-valued.
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.
Hi @mogsie
Thank you a lot for your help on those tests !!!
That is amazing !
Since it is just a nit wdyt about we get this one merged and then you push a follow up just to improve this part?
Would you like to contribute with the above enhancement?
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.
Sure. I can follow up on this. Should I make a separate issue for 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.
Feel free tor raise the issue, but if you will work on that asap. just push the PR is fine.
I am moving forward with this because:
|
…e cronjob tutorial to clarify its usage and validate the changes (kubernetes-sigs#4130) improve the webhook tests by adding examples In this PR we are improving the webhook tests by adding further info and examples for the users. Either we are implementing them further for the cronjob tutorial as an example and to help us to validate and spot issues on related areas when/if we need to change them
In this PR we are improving the webhook tests by adding further info and examples for the users. Either we are implementing them further for the cronjob tutorial as an example and to help us to validate and spot issues on related areas when/if we need to change them
Closes: #3454