feat: enhances FeatureTracker with spec #17
Conversation
apis/features/v1/features_types.go
Outdated
| ComponentType = "Component" | ||
| DSCIType = "DSCI" |
There was a problem hiding this comment.
How about improving this slightly and make sub-type for string?
type OwnerType string
const (
ComponentOwner OwnerType = "Component"
DSCIOwner OwnerType = "DSCI"
)
and use this type instead? That would make it a bit more focused and type-safe. We already use something similar across the codebase - ManagementState
There was a problem hiding this comment.
On that note - maybe a follow-up PR to our "feature" branch could similarly make the condition phase its own type. Somehow we missed that opportunity in the original PR.
| testFeature, err = feature.CreateFeature(testFeatureName). | ||
| For(dsciSpec). | ||
| For(dsciSpec, origin). |
There was a problem hiding this comment.
Is that right? Origin is of a type DSCI here?
There was a problem hiding this comment.
linter got me there -__- fixed, here 103593f
| @@ -535,6 +546,14 @@ func newDSCInitializationSpec(ns string) *dscv1.DSCInitializationSpec { | |||
| return &spec | |||
| } | |||
|
|
|||
| func newDefaultOrigin() featurev1.Origin { | |||
There was a problem hiding this comment.
Maybe we can extract this and the other newOrigin to somewhere where we share test fixtures and use it across instead of duplicating the code? I am also wondering if Default is self-explanatory here.
components/dashboard/dashboard.go
Outdated
| @@ -217,7 +218,10 @@ func (d *Dashboard) Cleanup(cli client.Client, dscispec *dsciv1.DSCInitializatio | |||
| } | |||
|
|
|||
| if shouldConfigureServiceMesh { | |||
| serviceMeshInitializer := feature.NewFeaturesInitializer(dscispec, d.defineServiceMeshFeatures(dscispec)) | |||
| serviceMeshInitializer := feature.NewFeaturesInitializer(dscispec, d.defineServiceMeshFeatures(dscispec), featurev1.Origin{ | |||
There was a problem hiding this comment.
I would extract it to it's own origin variable for better readability. Also, if the only reason to pass it is to use it in .For of the defineXYFeatures I think it could be a parameter of this function instead. That would also remove a need for keeping it as a field of FeaturesInitializer.
There was a problem hiding this comment.
done here:
0571826
learned about go's closures for this : ) , didn't know how to accomplish this at first which is what led to adding origin to the featureinitializer
| origin := featurev1.Origin{ | ||
| Type: featurev1.DSCIType, | ||
| Name: instance.Name, | ||
| } | ||
| defineServiceMesh := func(s *feature.FeaturesInitializer) error { | ||
| return defineServiceMeshFeatures(s, origin) | ||
| } | ||
|
|
||
| serviceMeshInitializer := feature.NewFeaturesInitializer(&instance.Spec, defineServiceMesh) |
components/kserve/kserve.go
Outdated
| origin := featurev1.Origin{ | ||
| Type: featurev1.ComponentType, | ||
| Name: k.GetComponentName(), | ||
| } | ||
| configureServerless := func(s *feature.FeaturesInitializer) error { | ||
| return k.configureServerlessFeatures(s, origin) | ||
| } | ||
|
|
||
| serverlessInitializer := feature.NewFeaturesInitializer(instance, configureServerless) |
There was a problem hiding this comment.
How about aligning it to how it's done in dashboard.go?
That would mean:
origin := featurev1.Origin{
Type: featurev1.ComponentType,
Name: k.GetComponentName(),
}
serverlessInitializer := feature.NewFeaturesInitializer(instance, k.configureServerlessFeatures(instance, origin))and chaging configureServerlessFeatures to:
func (k *Kserve) configureServerlessFeatures(dscispec *dsci.DSCInitializationSpec, origin featurev1.Origin) feature.DefinedFeatures {
return func(initializer *feature.FeaturesInitializer) error {
// old code
}
}
bartoszmajsak
changed the title
cam-garrison
merged commit 3043c64
into
maistra:service-mesh-integration
* feat: enhances FeatureTracker with spec (#17) * initial add tracker spec * update tests, update crd * add omitempty to origin struct * undo accidental tag change * re add empty line * move pointer operator * add testing * lint * re-lint changes * add ownertype, move newOrigin() to shared util * Update apis/features/v1/features_types.go Co-authored-by: Bartosz Majsak <[email protected]> * remove origin from featureinitializer * modify kserve sm step to match dashboard's * make dsci servicemesh setup like dashboard's * fix merge issues, lint --------- Co-authored-by: Bartosz Majsak <[email protected]> * restore testing mistakenly removed in merge * satisfy linter post merge conflicts * fix linter post merge * fix post merge issue * split For() into With + DefinedBy * rename origin to source, definedby to from --------- Co-authored-by: Bartosz Majsak <[email protected]>
Adds origin and appNamespace fields to the
FeatureTrackerSpec.This requires passing in additional information into For() when creating a feature - namely the Origin struct which captures if the Feature is being created by a DSCI or a component of the DSC as well as the name of said DSCI or component.
This PR also adds some basic testing.