Add Simulation API types #110
Conversation
| // +kubebuilder:default=AcceptingChanges | ||
| // +kubebuilder:validation:Enum=AcceptingChanges;Complete;Terminated | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Complete' ? self == 'Complete' || self == 'Terminated' : true",message="A complete Simulation can only be terminated" | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Terminated' ? self == oldSelf : true",message="A Simulation can't be un-terminated" |
There was a problem hiding this comment.
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Terminated' ? self == oldSelf : true",message="A Simulation can't be un-terminated" | |
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Terminated' ? self == oldSelf : true",message="A Simulation can't be un-terminated" | |
| // +optional |
There was a problem hiding this comment.
Can you clarify why you're suggesting making this optional? It's not supposed to be an optional field.
There was a problem hiding this comment.
It has a default value, so user doesn't have to provide it meaning it is optional.
But probably having a default is different than being optional 🤔
| // A CompletionCriterion specifies when a simulation is complete. | ||
| type CompletionCriterion struct { | ||
| // Type of criterion. | ||
| // +kubebuilder:validation:Enum=Duration |
There was a problem hiding this comment.
| // +kubebuilder:validation:Enum=Duration | |
| // +kubebuilder:default=Duration | |
| // +kubebuilder:validation:Enum=Duration |
There was a problem hiding this comment.
I'm not sure how I feel about defaulting this to Duration.
Right now it's the only choice, but I think we'll add more values in future. I don't think we'd make duration the default once there are more options - there wouldn't be a default. So feel it could be surprising to users to make it the default now, but then not in future. It's a subtle difference, but I think I prefer requiring folks to explicitly specify the one allowed value.
|
|
||
| // CompletionCriteria specify how Spaces should determine when the | ||
| // simulation is complete. If any of the criteria are met, Spaces will set | ||
| // the Simulation's desired state to complete. Omit the criteria if you want |
There was a problem hiding this comment.
If any of the criteria are met, Spaces will set the Simulation's desired state to complete.
Will it edit spec.desiredState ?
What if the user already set it to Complete or Terminated in the beginning? Would CompletionCriteria be ignored then? Any practical reason for someone to set it any value other than AcceptingChanges ? 🤔
There was a problem hiding this comment.
Will it edit spec.desiredState ?
Yes, it will.
What if the user already set it to Complete or Terminated in the beginning?
Yes, if the simulation is already complete (or terminated) the completion criteria are ignored.
Any practical reason for someone to set it any value other than AcceptingChanges ?
You mean spec.desiredState? If so:
-
You'd set
spec.desiredStateto Complete to manually mark the simulation complete, and thus trigger detection of the changes that happened while it was accepting changes. In this case you'd probably leave the completion criteria empty. Though you could also set the completion criteria to say 10 minutes but then later decide to manually mark the simulation complete (by changingspec.desiredStateafter 5 minutes. -
You'd set
spec.desiredStateto Terminated to terminate the simulation. This is the only way a simulation can be terminated. (A terminated simulation no longer has a control plane, but still exists so you can see the change summary in status.)
I'm open to updating the comment if there's a relatively succinct way to make this clearer.
Signed-off-by: Nic Cope <[email protected]>
Description of your changes
These types are in service of an upcoming private preview Upbound feature.
I have:
make reviewableto ensure this PR is ready for review.Addedbackport release-x.ylabels to auto-backport this PR, as appropriate.How has this code been tested
These types have been tested with unit and E2E tests on an Upbound internal PR.