Skip to content

Add flavor extra_specs#817

Open
chess-knight wants to merge 1 commit into
k-orc:mainfrom
chess-knight:flavor-extra-specs
Open

Add flavor extra_specs#817
chess-knight wants to merge 1 commit into
k-orc:mainfrom
chess-knight:flavor-extra-specs

Conversation

@chess-knight

Copy link
Copy Markdown

Add Flavor extra_specs.

Also, create common type ExtraSpec and use it for the Flavor and VolumeType.

Closes #738

Signed-off-by: Roman Hros <roman.hros@dnation.cloud>
@github-actions github-actions Bot added the semver:major Breaking change label Jun 11, 2026

@eshulman2 eshulman2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR, generally LGTM except for the missing interface aliases.

return deletes
}

func (actuator flavorActuator) GetResourceReconcilers(ctx context.Context, orcObject orcObjectPT, osResource *osResourceT, controller generic.ResourceController) ([]resourceReconciler, progress.ReconcileStatus) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

flavorActuator now implements ReconcileResourceActuator via GetResourceReconcilers, but the type alias and compile-time assertion are missing. Compare securitygroup/actuator.go which has:

reconcileResourceActuator = interfaces.ReconcileResourceActuator[orcObjectPT, orcObjectT, osResourceT]
// ...
var _ reconcileResourceActuator = securityGroupActuator{}

The resourceReconciler alias was added, but reconcileResourceActuator wasn't. Please add:

reconcileResourceActuator = generic.ReconcileResourceActuator[orcObjectPT, orcObjectT, osResourceT]
var _ reconcileResourceActuator = flavorActuator{}

}
}

func TestExtraSpecUpdates(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice to have: would be great to add a test covering the entire logic of the update function eg. what happens if update succeed but delete fails, etc

// +kubebuilder:validation:MaxItems:=128
// +listType=atomic
// +optional
ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The []ExtraSpec slice has no uniqueness constraint on name. If a user specifies the same name twice, extraSpecsToMap silently uses the last value - no error or warning at admission time.

A CEL validation rule on the field would catch this at admission time:

// +kubebuilder:validation:XValidation:rule="self.all(x, self.filter(y, y.name == x.name).size() == 1)",message="extraSpecs names must be unique"
ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

// +listType=atomic
// +optional
ExtraSpecs []VolumeTypeExtraSpec `json:"extraSpecs,omitempty"`
ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above:

The []ExtraSpec slice has no uniqueness constraint on name. If a user specifies the same name twice, extraSpecsToMap silently uses the last value - no error or warning at admission time.

A CEL validation rule on the field would catch this at admission time:

// +kubebuilder:validation:XValidation:rule="self.all(x, self.filter(y, y.name == x.name).size() == 1)",message="extraSpecs names must be unique"
ExtraSpecs []ExtraSpec `json:"extraSpecs,omitempty"`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:major Breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nova: Flavor extra_specs

2 participants