Rename Infrastructure.CloudProvider to Platform #149
Conversation
openshift-ci-robot
requested review from
abhinavdahiya,
deads2k,
derekwaynecarr,
flaper87,
rajatchopra,
smarterclayton and
staebler
openshift-ci-robot
added
the
size/XS
|
The field name Platform is more synonymous with what installer installs. And clubbing that info into /approve |
config/v1/types_infrastructure.go
Outdated
|
|
||
| type InfrastructureStatus struct { | ||
| // cloudProvider is the IaaS provider that is running the cluster. | ||
| // Platform is the IaaS provider that is running the cluster. |
There was a problem hiding this comment.
It's not actually an IaaS provider. Let's fix the message too here. (remember, you must use lower case names for fields)
platform is the underlying infrastructure provider for the cluster. This value controls whether infrastructure automation such as service load balancers, dynamic volume provisioning, machine creation and deletion, and other integrations are enabled. If unset, no infrastructure automation is enabled.
However, we also need to correct the constants to be consistent with our API rules (should be AWS, OpenStack) and we should have an explicit None option. That can be a follow up, but must happen before 4.0.
There was a problem hiding this comment.
Eventually we also need to document the rules for changes are (are users allowed to change this, what happens if it is wrong, etc).
There was a problem hiding this comment.
I took a stab at implementing this feedback, PTAL.
There was a problem hiding this comment.
@smarterclayton ping, would love to wrap this up today if possible
openshift-ci-robot
added
size/S
ironcladlou
force-pushed
the
cloud-provider-refactor
branch
from
be384e8 to
8d79f80
Compare
|
|
||
| // openStackPlatform represents OpenStack. | ||
| OpenStackPlatform PlatformType = "openstack" | ||
|
|
There was a problem hiding this comment.
Can we add libvirt too? We already at least have machine management
ironcladlou
force-pushed
the
cloud-provider-refactor
branch
from
8d79f80 to
586402d
Compare
openshift-ci-robot
added
size/M
|
ping @deads2k @smarterclayton |
config/v1/types_infrastructure.go
Outdated
| LibvirtPlatform PlatformType = "libvirt" | ||
|
|
||
| // nonePlatform means there is no infrastructure provider. | ||
| NonePlatform PlatformType = "none" |
There was a problem hiding this comment.
We should be using Kube style constants unless we have a reason not to. None, LibVirt, AWS, OpenStack
There was a problem hiding this comment.
Fixed, thanks for the feedback
smarterclayton
left a comment
smarterclayton
left a comment
There was a problem hiding this comment.
One comment about constants
Rename the Infrastructure CloudProvider field to Platform in an effort to more clearly establish the value is not directly mapped to Kubernetes cloud provider constants. Kubernetes cloud provider names may be derived from the value.
ironcladlou
force-pushed
the
cloud-provider-refactor
branch
from
586402d to
410d994
Compare
|
All feedback addressed, PTAL |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, ironcladlou, smarterclayton 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 |
openshift-ci-robot
added
the
approved
|
/retest |
5 participants
Rename the Infrastructure CloudProvider field to Platform in an effort to more
clearly establish the value is not directly mapped to Kubernetes cloud provider
constants. Kubernetes cloud provider names may be derived from the value.
/cc @openshift/sig-network-edge @openshift/installer @derekwaynecarr @deads2k @rajatchopra @flaper87 @smarterclayton @abhinavdahiya @staebler