feat: Validate AuthProxyWorkload spec.selector field#209
Conversation
| wantUpdateValid bool | ||
| } | ||
|
|
||
| data := []*testcase{ |
There was a problem hiding this comment.
It's fine to do it this way, but it's a little more common to just declare an anonymous struct inline like so:
data := []struct{
desc string
spec cloudsqlapi.AuthProxyWorkloadSpec
}{
{
// ... data here
}
}| return nil | ||
| } | ||
|
|
||
| func (r *AuthProxyWorkload) Validate() error { |
There was a problem hiding this comment.
Since we're testing through ValidateCreate and ValidateUpdate, does this need to be exported?
There was a problem hiding this comment.
Probably not. I'll hide it.
| allErrs = append(allErrs, validation.ValidateLabelName(r.Name, field.NewPath("metadata", "name"))...) | ||
| allErrs = append(allErrs, validateSpec(&r.Spec, field.NewPath("spec"))...) | ||
|
|
||
| if len(allErrs) == 0 { |
There was a problem hiding this comment.
This should be reversed. Indent the error flow.
|
|
||
| allErrs = append(allErrs, validateWorkload(&spec.Workload, f.Child("workload"))...) | ||
|
|
||
| return allErrs |
There was a problem hiding this comment.
We can just inline allErrs here.
There was a problem hiding this comment.
Fixed. I got rid of validateSpec() entirely.
| desc string | ||
| spec cloudsqlapi.AuthProxyWorkloadSpec | ||
| wantCreateValid bool | ||
| wantUpdateValid bool |
| { | ||
| desc: "happy path labels", | ||
| spec: cloudsqlapi.AuthProxyWorkloadSpec{ | ||
| Workload: cloudsqlapi.WorkloadSelectorSpec{Kind: "Deployment", Selector: &v1.LabelSelector{MatchLabels: map[string]string{"app": "sample"}}}, |
There was a problem hiding this comment.
nit: let's try to wrap at 100 or even 80 characters.
| wantUpdateValid: true, | ||
| }, | ||
| { | ||
| desc: "invalid labels and name both set", |
There was a problem hiding this comment.
let's try to phrase this as "if name and levels are both set, error" or use some other kind of descriptive message that makes it easy to understand at a glance.
| wantUpdateValid: true, | ||
| }, | ||
| { | ||
| desc: "happy path update", |
There was a problem hiding this comment.
Does this test case still belong here?
| data := []struct { | ||
| desc string | ||
| spec cloudsqlapi.AuthProxyWorkloadSpec | ||
| wantCreateValid bool |
There was a problem hiding this comment.
I simplified to "wantValid", which makes more sense to me when I read the test code.
| desc string | ||
| spec cloudsqlapi.AuthProxyWorkloadSpec | ||
| oldSpec cloudsqlapi.AuthProxyWorkloadSpec | ||
| wantCreateValid bool |
| }, | ||
| wantCreateValid: true, | ||
| wantUpdateValid: true, | ||
| }, |
There was a problem hiding this comment.
Should we add some failure cases here?
There was a problem hiding this comment.
Failure cases are coming in future PRs.
hessjcg
left a comment
hessjcg
left a comment
There was a problem hiding this comment.
Updated naming semantics in authproxyworkload_test.go
| data := []struct { | ||
| desc string | ||
| spec cloudsqlapi.AuthProxyWorkloadSpec | ||
| wantCreateValid bool |
There was a problem hiding this comment.
I simplified to "wantValid", which makes more sense to me when I read the test code.
| desc string | ||
| spec cloudsqlapi.AuthProxyWorkloadSpec | ||
| wantCreateValid bool | ||
| wantUpdateValid bool |
| { | ||
| desc: "happy path labels", | ||
| spec: cloudsqlapi.AuthProxyWorkloadSpec{ | ||
| Workload: cloudsqlapi.WorkloadSelectorSpec{Kind: "Deployment", Selector: &v1.LabelSelector{MatchLabels: map[string]string{"app": "sample"}}}, |
| }, | ||
| wantCreateValid: true, | ||
| wantUpdateValid: true, | ||
| }, |
There was a problem hiding this comment.
Failure cases are coming in future PRs.
Features - Add new field RolloutStrategy control automatic rollout (#202) (090b88d) - Add new terraform project for e2e test resources (#181) (0140592) - Add script to run terraform with input validation. (#182) (857444a) - Add support for Unix sockets. (#205) (8177a35), closes #47 - Add telemetry settings to configure health check port (#210) (3ede42d) - Add the e2e test job for Cloud Build (#184) (dc2990c) - Automatic changes to workloads when an AuthProxyWorload is deleted (#200) (e11caed) - Automatically trigger pod rollout for appsv1 resources when AuthProxyWorkload changes. (#197) (3b0359b) - Separate terraform for project setup and permissions (#179) (8f43657) - Validate AuthProxyWorkload spec.selector field (#209) (98c460b) - Validate AuthProxyWorkload updates to prevent changes to the workload selector. (#211) (4304283) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Release PR Generate Bot action release-please[bot] <release-please[bot]@users.noreply.github.com>
2 participants
Ensure that the workload selector field is valid. Either
nameorselectormust be set, but not both.Related to #36