-
Notifications
You must be signed in to change notification settings - Fork 48
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃尡Add checks in ValidCreate and ValidUpdate in hbmmt webhook #1319
base: main
Are you sure you want to change the base?
Conversation
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.
looks good so far. You can see in the examples I mentioned in the issue how they did it. You can maybe find an approach with a utility function that allows you to not copy paste the code in the two places
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.
cool! Small thing about consistency: Can you name everything you added with "HCloud"? Capital "C"? We usually do that
removed it |
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.
looks good from my side! Have you already tested the new webhooks for template objects?
You can also write unit tests similar to
var _ = Describe("HCloudMachine validation", func() { |
241c620
to
1e32559
Compare
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.
lgtm. Looks nice. Thanks! You can squash! make sure to choose a good commit title AND message where you describe what you do
Added validation checks for hbmm , hbmmt , hcloudmachine and hcloud machinetemplate webhooks . Also added the unit tests for the same
25ccb55
to
180a275
Compare
}) | ||
It("should validate update", func() { | ||
newhbmmt := hbmmt.DeepCopy() | ||
warnings, err := hbmmtwebhook.ValidateUpdate(ctx, oldhbmmt, newhbmmt) |
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 this test doesn't make sense. oldhbmmt is always nil!
@@ -247,5 +247,59 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() { | |||
}, timeout, time.Second).Should(BeTrue()) | |||
}) | |||
}) | |||
Context("HCloudMachineTemplate Webhook Validation", func() { |
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.
the tests here should work with envtest. This means that you create an object and depending on whether you want it to succeed or fail, you expect an error on creation or not. Same with update. For update, you need to first create it, then fetch it, then update it, and expect either an error or not
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1165
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs: