-
Notifications
You must be signed in to change notification settings - Fork 80
implement trait.ApplyTo through AppConfig validating webhook #310
Conversation
) | ||
|
||
func TestCheckComponentVersionEnabled(t *testing.T) { |
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.
why delete this test?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't return all errors at once, please directly return this error.
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.
got it.
} | ||
} | ||
return false, nil | ||
// remove nil error | ||
return allErrors |
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.
just return nil, if no errors, allErrors cause confusion
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.
got it.
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does it mean?
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.
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
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 after nit and rebase
for _, c := range v.validatingComps { | ||
for _, t := range c.validatingTraits { | ||
if t.traitDefinition.Spec.RevisionEnabled { | ||
goto Validate |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 <[email protected]>
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
ValidatingAppConfig
typed struct to prepare all data before validationSigned-off-by: roy wang [email protected]