OpenStack: Rename lbFloatingIP to apiFloatingIP #4244
Conversation
|
/test e2e-openstack |
Fedosin
force-pushed
the
api_floating_ip
branch
from
cf6b87f
to
92323b6
Compare
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/approve |
|
/test e2e-aws |
| // ConvertNetworking upconverts deprecated fields in networking | ||
| func ConvertNetworking(config *types.InstallConfig) { | ||
| // convertNetworking upconverts deprecated fields in networking | ||
| func convertNetworking(config *types.InstallConfig) { |
There was a problem hiding this comment.
I dunno why this was public before, but making it private seems like an orthogonal change that should go in its own PR.
There was a problem hiding this comment.
okay, I'll propose another PR to fix this.
| // convertOpenStack upconverts deprecated fields in the OpenStack platform. | ||
| func convertOpenStack(config *types.InstallConfig) { | ||
| // LbFloatingIP has been renamed to APIFloatingIP | ||
| if config.Platform.OpenStack.DeprecatedLbFloatingIP != "" && config.Platform.OpenStack.APIFloatingIP == "" { |
There was a problem hiding this comment.
Seems like it should be a fatal error to have both set. Also, probably worth warn-level logging when you do shuffle the value over, so folks using the old value are more likely to hear about the deprecation and update their generators.
There was a problem hiding this comment.
this will go in a separate PR for all deprecated values. we have a dedicated jira card for this.
| func convertOpenStack(config *types.InstallConfig) { | ||
| // LbFloatingIP has been renamed to APIFloatingIP | ||
| if config.Platform.OpenStack.DeprecatedLbFloatingIP != "" && config.Platform.OpenStack.APIFloatingIP == "" { | ||
| config.Platform.OpenStack.APIFloatingIP = config.Platform.OpenStack.DeprecatedLbFloatingIP |
There was a problem hiding this comment.
I'd expect to clear the old property when you shuffle the value over.
There was a problem hiding this comment.
I think we should keep it to show a warning message later, during install config validation.
There was a problem hiding this comment.
I'd expect to clear the old property when you shuffle the value over.
Don't reset user set values, if a user set the deprecated value, we can use that to set the new values but do not modify the value set by the user.
Fedosin
force-pushed
the
api_floating_ip
branch
from
92323b6
to
85e0297
Compare
Fedosin
force-pushed
the
api_floating_ip
branch
from
85e0297
to
308b5ea
Compare
| @@ -106,13 +106,13 @@ func validatePlatformFlavor(p *openstack.Platform, ci *CloudInfo, fldPath *field | |||
| } | |||
|
|
|||
| func validateFloatingIPs(p *openstack.Platform, ci *CloudInfo, fldPath *field.Path) (allErrs field.ErrorList) { | |||
There was a problem hiding this comment.
can we add a validation that makes sure people don't set both with different values.
There was a problem hiding this comment.
I added a check that returns an error if both values are set regardless of whether they are equal or not. I.e. "you cannot specify lbFloatingIP and apiFloatingIP in the install config together".
LGTM, only one change requested. |
Fedosin
force-pushed
the
api_floating_ip
branch
from
308b5ea
to
68a69b7
Compare
|
/approve |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, mandre, pierreprinetti 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 |
|
/lgtm |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel nope :) |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@Fedosin: 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
cf. openshift/installer#4244 Implements OSASINFRA-2157
We deprecated `lbFloatingIP` in favor of `apiFloatingIP` in openshift/installer#4244. Fixes OSASINFRA-2157
/label platform/openstack