Refactor UpgradeStrategy to UpgradeSpec.Type #2678
Conversation
Signed-off-by: ryanaoleary <[email protected]>
| // UpgradeStrategy represents the strategy used when upgrading the RayService. Currently supports `NewCluster` and `None` | ||
| UpgradeStrategy *RayServiceUpgradeStrategy `json:"upgradeStrategy,omitempty"` | ||
| // UpgradeSpec defines the scaling policy used when upgrading the RayService. | ||
| UpgradeSpec *RayServiceUpgradeSpec `json:"upgradeSpec,omitempty"` |
There was a problem hiding this comment.
spec.upgradeStrategy.type instead of spec.upgradeSpec.type?
There was a problem hiding this comment.
Sure that sounds more clear, RayServiceUpgradeStrategy was already in use to define the string constant, but I went ahead and changed that field to RayServiceUpgradeType instead. 97816e8
Signed-off-by: ryanaoleary <[email protected]>
|
@ryanaoleary can you fix the CI failure? Thanks! |
Signed-off-by: ryanaoleary <[email protected]>
Done in 2a0cf6e. |
| if upgradeStrategy := rayService.Spec.UpgradeStrategy; upgradeStrategy != nil { | ||
| if *upgradeStrategy != rayv1.NewCluster && *upgradeStrategy != rayv1.None { | ||
| return fmt.Errorf("spec.UpgradeStrategy value %s is invalid, valid options are %s or %s", *upgradeStrategy, rayv1.NewCluster, rayv1.None) | ||
| if rayService.Spec.UpgradeStrategy == nil { |
There was a problem hiding this comment.
Can we merge these two if statements into one? The current control flow can easily skip some checks we might add in the future. For example, if UpgradeStrategy is nil, the "check others" step will be skipped.
if rayService.Spec.UpgradeStrategy == nil {
return nil
}
// check upgradeType
// check othersThere was a problem hiding this comment.
maybe leave this as a good first issue for new contributors.
Why are these changes needed?
This PR changes
UpgradeStrategyto a fieldTypein a newUpgradeSpecfield underRayServiceSpec. This will make it easier to add other upgrade related fields to the RayService CR in the future.Related issue number
Checks