Skip to content
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

feature(webhook): add BackendRuntimeConfig resources validation #170

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

googs1025
Copy link
Contributor

/kind feature

What this PR does / why we need it

  • add validation for webhook

Which issue(s) this PR fixes

Fixes #135

Special notes for your reviewer

None

Does this PR introduce a user-facing change?

None

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Sep 20, 2024
@googs1025
Copy link
Contributor Author

Maybe we need to do some unit testing on the webhook, but this will be a lot of work

@googs1025
Copy link
Contributor Author

/kind feature

@InftyAI-Agent InftyAI-Agent added feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Sep 20, 2024
@kerthcet
Copy link
Member

Webhook tests usually covered by the integration test, and we do lack some kind of tests like updating validation.

Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an integration test under /test/integraton/webhook/playground_test.go

pkg/webhook/playground_webhook.go Outdated Show resolved Hide resolved
@googs1025
Copy link
Contributor Author

Let's add an integration test under /test/integraton/webhook/playground_test.go

I will handle this tonight

@googs1025 googs1025 force-pushed the valid/resource branch 2 times, most recently from 5253a51 to bf20413 Compare September 21, 2024 11:32
@googs1025
Copy link
Contributor Author

Let's add an integration test under /test/integraton/webhook/playground_test.go

done. @kerthcet

@@ -69,6 +69,12 @@ var _ = ginkgo.Describe("Playground default and validation", func() {
},
failed: true,
}),
ginkgo.Entry("invalid BackendRuntime resources", &testValidatingCase{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add another example to cover the scenario: resources in limits but not in requests to make sure there's no panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kerthcet
Copy link
Member

/lgtm
/approve

Thanks! @googs1025

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 23, 2024
@InftyAI-Agent InftyAI-Agent merged commit 4c35e01 into InftyAI:main Sep 23, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add validation to Playground as the backendConfig's resource requests should not be greater than limits
3 participants