Add oVirt platform status #451
Conversation
Signed-off-by: Roy Golan <[email protected]>
openshift-ci-robot
added
the
size/S
|
Please link the enhancement to this PR so we can understand the impact this has on 4.3 |
Can you link the enhancement please |
|
/approve @abhinavdahiya PTAL |
There was a problem hiding this comment.
/approve
Leaving final call to @abhinavdahiya like Scott pointed out.
openshift-ci-robot
added
the
approved
| // by components inside the cluster, like kubelets using the infrastructure rather | ||
| // than Kubernetes networking. It is the IP that the Infrastructure.status.apiServerInternalURI | ||
| // points to. It is the IP for a self-hosted load balancer in front of the API servers. | ||
| APIServerInternalIP string `json:"apiServerInternalIP,omitempty"` |
There was a problem hiding this comment.
omitempty with string will cause they are not printed when empty, but it doesn't make any difference during parsing. I don't think we have clear guidance for it, though.
There was a problem hiding this comment.
omitempty for optional fields is often a good idea. Usually only if you want to distinguish undefined and zero value, you don't use omitempty. Though then these fields are also pointers to survive Golang roundtripping.
There was a problem hiding this comment.
https://github.com/openshift/api/blob/master/config/v1/doc.go#L5
all fields are kindof optional at package level, and this is similar to other structs, so following that seems fine to me.
| @@ -130,6 +130,10 @@ type PlatformStatus struct { | |||
| // OpenStack contains settings specific to the OpenStack infrastructure provider. | |||
| // +optional | |||
| OpenStack *OpenStackPlatformStatus `json:"openstack,omitempty"` | |||
|
|
|||
| // Ovirt contains settings specific to the oVirt infrastructure provider. | |||
| // +optional | |||
There was a problem hiding this comment.
@damemi one needs the kubebuilder optional tag as well, doesn't one?
There was a problem hiding this comment.
though it does not matter here as the default is optional already
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, rgolangh, sdodson, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add oVirt platform status
enhancement: openshift/enhancements#61