SPLAT-1410: Modified vSphere config provider to not lose AddressesFromPools when applying Failure Domains #273
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-1410 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 task to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
size/M
| } | ||
|
|
||
| // Set the network name for the device from FD. | ||
| networkSpec.Devices[0].NetworkName = topology.Networks[0] |
There was a problem hiding this comment.
while not allowed now, i could see us supporting possibly multiple network devices in the future. should we initialize the network spec with any topology networks we find?
There was a problem hiding this comment.
I can do this. I didn't like hard coding this, but we were before. I'll update this and test out.
|
/test all |
|
@vr4manta: This pull request references SPLAT-1410 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 task to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@vr4manta: This pull request references SPLAT-1410 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 task to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@vr4manta: This pull request references SPLAT-1410 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 task to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@vr4manta: This pull request references SPLAT-1410 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 task to target the "4.16.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
size/XL
vr4manta
force-pushed
the
SPLAT-1410
branch
2 times, most recently
from
6f36330
to
c686984
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
openshift-ci
bot
added
lgtm
|
/lgtm |
|
/retest |
|
/test unit |
|
Unit test failed, looks like the injection isn't quite working as expected? |
This works fine locally. I am not sure why it keeps failing in CI. |
|
/test unit |
Ok, i recreated the failure where static ip test fails. looking into fix now. (rebase was required) |
|
/retest-required |
|
@JoelSpeed The unit test are fixed. Ready for review. |
| }) | ||
|
|
||
| It("contains an AddressesFromPools block", func() { | ||
| Expect(providerConfig.providerConfig.Network.Devices[0].AddressesFromPools).To(Not(BeEmpty())) |
There was a problem hiding this comment.
Quite recently we started adding the optionalDescription in the Gomega assertions (e.g. in .To()).
So I'd be nice if all the assertions you are adding (e.g. .To(), .ToNot(), Should() etc.) have a failure description, so then if they fail they provide more context in the error: e.g.
| Expect(providerConfig.providerConfig.Network.Devices[0].AddressesFromPools).To(Not(BeEmpty())) | |
| Expect(providerConfig.providerConfig.Network.Devices[0].AddressesFromPools).To(Not(BeEmpty()), "expected AddressesFromPools to not be empty as a static IPPool has been configured") |
There was a problem hiding this comment.
sounds good. I"ll add this in.
|
|
||
| It("returns networking with AddressesFromPools and the configured network name from failure domain", func() { | ||
| expected, err := providerConfig.InjectFailureDomain(providerConfig.ExtractFailureDomain()) | ||
| Expect(err).To(BeNil()) |
There was a problem hiding this comment.
Here we normally prefer to use:
| Expect(err).To(BeNil()) | |
| Expect(err).To(Not(HaveOccurred())) |
As that provides a nicer output:
Unexpected error:
<*errors.errorString | 0xc00002c5d0>:
this is a very bad error
{
s: "this is a very bad error",
}
occurred
as opposed to:
Expected
<*errors.errorString | 0xc0000a0590>:
this is a very bad error
{
s: "this is a very bad error",
}
to be nil
There was a problem hiding this comment.
Makes sense. I'll update this as well.
…applying Failure Domains.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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 |
openshift-ci
bot
added
the
approved
|
@vr4manta: The following tests failed, say
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/test-infra repository. I understand the commands that are listed here. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
/test e2e-aws-ovn |
|
/retest-required |
openshift-merge-bot
bot
merged commit 20911c8
into
openshift:main
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-control-plane-machine-set-operator-container-v4.16.0-202402210139.p0.g20911c8.assembly.stream.el9 for distgit ose-cluster-control-plane-machine-set-operator. |
SPLAT-1410
Changes
Dependencies