-
Notifications
You must be signed in to change notification settings - Fork 585
Add rolloutStategy parameter to ImageRegistrySpec #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add rolloutStategy parameter to ImageRegistrySpec #560
Conversation
| // rolloutStrategy defines rollout strategy for the image registry | ||
| // deployment. | ||
| // +optional | ||
| RolloutStrategy string `json:"rolloutStrategy" protobuf:"bytes,15,opt,name=rolloutStrategy"` |
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.
@sttts can we make this a appsv1.DeploymentStrategyType [1]? Would we automatically get the validation pattern we want?
If not, then the validation pattern needs to be added as a comment tag - see [2] as an example.
[1] https://github.com/kubernetes/api/blob/master/apps/v1/types.go#L335
[2] https://github.com/openshift/api/blob/master/imageregistry/v1/types.go#L239
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.
done!
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.
@adambkaplan upstream types are coded (and validated) in Go. There is no connection to these validation here and those in Go land.
To allow us to define a rollout strategy for the image registry deployment, we create an additional parameter in ImageRegistrySpec.
Fedosin
left a comment
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.
Thanks for the review!
| // rolloutStrategy defines rollout strategy for the image registry | ||
| // deployment. | ||
| // +optional | ||
| RolloutStrategy string `json:"rolloutStrategy" protobuf:"bytes,15,opt,name=rolloutStrategy"` |
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.
done!
|
/assign @sttts |
|
/approve hold for @adambkaplan to have a last look. Remove hold at will. |
adambkaplan
left a comment
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.
/hold cancel
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, Fedosin, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
To allow us to define a rollout strategy for the image registry deployment, we create an additional parameter in ImageRegistrySpec.