Installer Dual Stack Enhancement #1731
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @sadasu |
openshift-ci
bot
requested review from
jhixson74,
JoelSpeed,
patrickdillon and
sadasu
- AWS - GCP - Azure The feature proposes changes to the: - cluster api providers - Openshift API - Openshift Installer
barbacbd
force-pushed
the
installer_dual_stack_enablement
branch
from
5b62c12 to
8e4e270
Compare
|
@barbacbd: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| variable. | ||
|
|
||
| ```go | ||
| // StackType: The stack type for the subnet. If set to IPV4_ONLY, new VMs in |
There was a problem hiding this comment.
In OpenShift, it is convention to use the JSON field name in the godoc instead of the Go field name (ref: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-json-field-names-in-godoc)
This is to make the end-user experience of using something like oc explain better by aligning the generated documentation with the field name the end-user sees.
| // StackType: The stack type for the subnet. If set to IPV4_ONLY, new VMs in | ||
| // the subnet are assigned IPv4 addresses only. If set to IPV4_IPV6, new VMs in | ||
| // the subnet can be assigned both IPv4 and IPv6 addresses. If not specified, | ||
| // IPV4_ONLY is used. This field can be both set at resource creation time and | ||
| // updated using patch. | ||
| // | ||
| // Possible values: | ||
| // "IPV4_IPV6" - New VMs in this subnet can have both IPv4 and IPv6 | ||
| // addresses. | ||
| // "IPV4_ONLY" - New VMs in this subnet will only be assigned IPv4 addresses. | ||
| // "IPV6_ONLY" - New VMs in this subnet will only be assigned IPv6 addresses. |
There was a problem hiding this comment.
The textual description and possible values feels repetitive. It might make sense to do something of the form:
stackType represents the stack type for the subnet.
Allowed values are IPV4_ONLY, IPV6_ONLY, IPV4_IPV6
When set to IPV4_ONLY ....
When set to IPV6_ONLY ...
...
| // "IPV6_ONLY" - New VMs in this subnet will only be assigned IPv6 addresses. | ||
| // +kubebuilder:validation:Enum=IPV4_ONLY;IPV4_IPV6;IPV6_ONLY | ||
| // +kubebuilder:default=IPV4_ONLY | ||
| // +optional |
There was a problem hiding this comment.
I would recommend explicitly calling out in the godoc that this field is optional towards the beginning of the godoc.
| // addresses. | ||
| // "IPV4_ONLY" - New VMs in this subnet will only be assigned IPv4 addresses. | ||
| // "IPV6_ONLY" - New VMs in this subnet will only be assigned IPv6 addresses. | ||
| // +kubebuilder:validation:Enum=IPV4_ONLY;IPV4_IPV6;IPV6_ONLY |
There was a problem hiding this comment.
Are ipv4 and ipv6 the only stack types possible for the subnet across all platforms? Could there be new ones added in the future that we would need to support?
If we could have to support various stack type combinations/permutations, an enum may not be best suited for this field. Imagining a case where you add a third stack type, you would have to expand your allowed enum values quite a bit. For example, adding stack type FOO would mean you have to add enum values of FOO_ONLY, IPV4_IPV6_FOO, IPV4_FOO, IPV6_FOO to capture all possible combinations of the values.
If that scenario is likely/possible, maybe it makes sense to make this a unique list of values where the type used in the slice has an enum validation in place that forces values to only ever be something like IPV4, IPV6, etc.?
There was a problem hiding this comment.
To further clarify, instead of having to do:
...
stackType: IPV4_IPV6
...A user would do:
...
stackType:
- IPV4
- IPV6
...| // +kubebuilder:validation:Enum=IPV4_ONLY;IPV4_IPV6;IPV6_ONLY | ||
| // +kubebuilder:default=IPV4_ONLY | ||
| // +optional | ||
| StackType string `json:"stackType,omitempty"` |
There was a problem hiding this comment.
In OpenShift, it is convention for enumerated fields to have a distinct type. Instead of using a string type here, consider creating a type alias to a string and creating some constants for the allowed values like:
type StackType string
const (
StackTypeIPV4 = "IPV4"
...
)
The dual stack enablement enhancement for:
The feature proposes changes to the: