SPLAT-1272: create fixtures for Nutanix failure domains support#304
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
yanhua121
force-pushed
the
nutanix-failuredomains
branch
from
5e178e7 to
c621225
Compare
openshift-merge-robot
removed
the
needs-rebase
yanhua121
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@yanhua121: This pull request references SPLAT-1272 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.15.0" version, but no target version was set. DetailsIn response to this:
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/test-infra repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Looks good, just one concern on the machine provider config builder
| // withFailureDomains sets the withFailureDomains field with the input value. | ||
| func (n *NutanixMachineProviderConfigBuilder) WithFailureDomains(withFailureDomains bool) *NutanixMachineProviderConfigBuilder { | ||
| n.withFailureDomains = withFailureDomains | ||
| return n | ||
| } | ||
|
|
||
| // WithFailureDomainIndex sets the failureDomainIndex field with the input value. | ||
| func (n *NutanixMachineProviderConfigBuilder) WithFailureDomainIndex(index int32) *NutanixMachineProviderConfigBuilder { | ||
| n.failureDomainIndex = index | ||
| return n | ||
| } |
There was a problem hiding this comment.
This should take a name to a failure domain rather than an index, this should not rely on the infrastructure object since that can be created with custom failure domains
That or it needs to be passed the failure domain from an external infrastructure object, but it definitely should not be defaulting like that
There was a problem hiding this comment.
Changed to allow to set the failureDomains data externally with the "withFailureDomains()" function.
yanhua121
force-pushed
the
nutanix-failuredomains
branch
2 times, most recently
from
8a27dc1 to
2a59562
Compare
|
/retest-required |
|
/assign @elmiko |
yanhua121
force-pushed
the
nutanix-failuredomains
branch
from
2a59562 to
3991b9c
Compare
|
@elmiko @JoelSpeed The review comments are addressed. Please take another look. thanks. |
elmiko
left a comment
elmiko
left a comment
There was a problem hiding this comment.
this looks better to me, i'm happy with this. i'd like @JoelSpeed to give another review tomorrow.
/lgtm
|
Ok thanks, this alleviates my concern about constructing the infra within the /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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
bot
added
the
approved
|
/override ci/prow/e2e-vsphere-operator These changes do not affect the E2E in any way |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-vsphere-operator DetailsIn response to this:
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/test-infra repository. |
|
@yanhua121: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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/test-infra repository. I understand the commands that are listed here. |
5 participants
SPLAT-1272: create fixtures for Nutanix failure domains support