implement trait.ApplyTo through AppConfig validating webhook #310
Conversation
| ) | ||
|
|
||
| func TestCheckComponentVersionEnabled(t *testing.T) { |
There was a problem hiding this comment.
It's moved to here and refactored.
| // get workload definition | ||
| wlDef, err := util.FetchWorkloadDefinition(ctx, c, dm, &wl) | ||
| if err != nil { | ||
| allErrors = append(allErrors, errors.Wrapf(err, |
There was a problem hiding this comment.
we don't return all errors at once, please directly return this error.
| } | ||
| } | ||
| return false, nil | ||
| // remove nil error | ||
| return allErrors |
There was a problem hiding this comment.
just return nil, if no errors, allErrors cause confusion
|
|
||
| // WorkloadNamePath indicates field path of workload name | ||
| WorkloadNamePath = "metadata.name" | ||
| ) | ||
|
|
||
| var appConfigResource = v1alpha2.SchemeGroupVersion.WithResource("applicationconfigurations") | ||
|
|
||
| // AppConfigValidator provides functions to validate ApplicationConfiguration | ||
| type AppConfigValidator interface { | ||
| Validate(context.Context, client.Client, discoverymapper.DiscoveryMapper, ValidatingAppConfig) []error |
There was a problem hiding this comment.
I see most cases only ValidatingAppConfig is used, why don't make it more clean like:
| Validate(context.Context, client.Client, discoverymapper.DiscoveryMapper, ValidatingAppConfig) []error | |
| Validate(context.Context, ValidatingAppConfig) []error |
There was a problem hiding this comment.
Maybe ValidateFunc needs to use client&dm in its own logic in the future, even though current ValidateFuncs don't need because prepared data is sufficient for them.
| // ValidatingHandler handles CloneSet | ||
| type ValidatingHandler struct { | ||
| Client client.Client | ||
| Mapper discoverymapper.DiscoveryMapper | ||
|
|
||
| // Decoder decodes objects | ||
| Decoder *admission.Decoder | ||
|
|
||
| DefaultValidators []AppConfigValidator |
There was a problem hiding this comment.
| DefaultValidators []AppConfigValidator | |
| Validators []AppConfigValidator |
I don't see all of them are defaults, if so, who're not default ones?
| for _, t := range c.validatingTraits { | ||
| if len(t.traitDefinition.Spec.AppliesToWorkloads) == 0 { | ||
| // AppliesToWorkloads is empty, the trait can be applied to ANY workload | ||
| continue |
There was a problem hiding this comment.
Both empty and "*" cases.
Don't forget *
| for _, applyTo := range t.traitDefinition.Spec.AppliesToWorkloads { | ||
| if workloadType == applyTo || | ||
| workloadDefRefName == applyTo || | ||
| workloadGroup == applyTo { |
There was a problem hiding this comment.
if the case is group, it should be like *.<group>, don't miss the *. prefix
| for _, c := range v.validatingComps { | ||
| workloadType := c.component.GetLabels()[oam.WorkloadTypeLabel] | ||
| workloadDefRefName := c.workloadDefinition.Spec.Reference.Name | ||
| // TODO version of workloadDefRef should be taken into account |
There was a problem hiding this comment.
Since definition is allowed to have more than one version, maybe ApplyTo is possible to indicate a specific version of WorkloadDefinition in the future.
552b152
to
1cbdcd3
Compare
| for _, c := range v.validatingComps { | ||
| for _, t := range c.validatingTraits { | ||
| if t.traitDefinition.Spec.RevisionEnabled { | ||
| goto Validate |
There was a problem hiding this comment.
using break is enough in this case
| for _, c := range v.validatingComps { | ||
| workloadType := c.component.GetLabels()[oam.WorkloadTypeLabel] | ||
| workloadDefRefName := c.workloadDefinition.Spec.Reference.Name | ||
| // TODO version of workloadDefRef should be taken into account |
There was a problem hiding this comment.
| // TODO version of workloadDefRef should be taken into account | |
| // TODO consider a CRD group could have multiple version and maybe we need to specify the minimum version here in the future |
implement trait.ApplyTo in validating webhook fix unit tests Signed-off-by: roy wang <seiwy2010@gmail.com>
1cbdcd3
to
b0f5416
Compare
fix #50 & fix #14
trait.ApplyTo can specify appliable workload in 3 formats: workloadType, workload DefinitionRef name, API Group.
e.g., ApplyTo: []string{
autoscaler,manuscalers.core.oam.dev,standard.oam.dev}to avoid repetitive GET/unmarshal operations in multiple validation funcs, use a new
ValidatingAppConfigtyped struct to prepare all data before validationSigned-off-by: roy wang seiwy2010@gmail.com