Check opts of cloud config file #48348
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @FengyunPan. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/L
FengyunPan
force-pushed
the
check-openstack-Opts
branch
from
9874ed5
to
ec616b2
Compare
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
FengyunPan
force-pushed
the
check-openstack-Opts
branch
3 times, most recently
from
9d8fdc9
to
8002880
Compare
|
/test pull-kubernetes-e2e-gce-etcd3 |
|
/assign |
|
/approve |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
k8s-github-robot
added
the
approved
|
/approve |
|
|
||
| // if need to create health monitor for Neutron LB, | ||
| // monitor-delay, monitor-timeout and monitor-max-retries should be set. | ||
| emptyDuration := new(MyDuration) |
There was a problem hiding this comment.
No need to create a reference to a MyDuration only to deref it again later. Just do: emptyDuration := MyDuration{} here and remove the asterisks below.
| delay := new(MyDuration) | ||
| delay.Duration = 60 * time.Second | ||
| timeout := new(MyDuration) | ||
| timeout.Duration = 30 * time.Second |
There was a problem hiding this comment.
Similar to earlier comment - these MyDuration objects should just be regular objects, not references.
delay := MyDuration{60 * time.Second}
timeout := MyDuration{30 * time.Second}| @@ -53,6 +53,11 @@ const ProviderName = "openstack" | |||
| var ErrNotFound = errors.New("Failed to find object") | |||
| var ErrMultipleResults = errors.New("Multiple results where only one expected") | |||
| var ErrNoAddressFound = errors.New("No address found for host") | |||
| var ErrNoSubnetId = errors.New("subnet-id not set in cloud provider config") | |||
There was a problem hiding this comment.
It's highly unlikely any caller is going to want to match against one of these different errors explicitly.
So instead of growing a huge list of pre-built errors here, delete these and just use in-line error strings in the function below:
if len(lbOpts.SubnetId) == 0 {
return fmt.Errorf("subnet-id not set in cloud provider config")
}
// etcFix kubernetes#48347 Check opts when register OpenStack CloudProvider rather than returning error when use opts to create/use cloud resource.
FengyunPan
force-pushed
the
check-openstack-Opts
branch
from
8002880
to
d2ebb60
Compare
k8s-github-robot
removed
the
lgtm
|
/test pull-kubernetes-federation-e2e-gce |
|
/test pull-kubernetes-kubemark-e2e-gce |
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FengyunPan, dims Associated issue: 48347 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 47234, 48410, 48514, 48529, 48348) |
Automatic merge from submit-queue (batch tested with PRs 47234, 48410, 48514, 48529, 48348) Check opts of cloud config file Fix kubernetes#48347 Check opts when register OpenStack CloudProvider rather than returning error when use opts to create/use cloud resource. **Release note**: ```release-note NONE ```
Fix #48347
Check opts when register OpenStack CloudProvider rather than
returning error when use opts to create/use cloud resource.
Release note: