Bug 1769879: SSC refractor #79
Conversation
This reverts commit 17be769.
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@iamemilio: This pull request references Bugzilla bug 1769879, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 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 kubernetes/test-infra repository. |
openshift-ci-robot
added
approved
iamemilio
force-pushed
the
ssc-refractor
branch
from
47f2b33 to
37f52fc
Compare
|
/retest |
|
|
||
| func NewInstanceServiceFromCloud(cloud clientconfig.Cloud, cert []byte) (*InstanceService, error) { | ||
| clientOpts := new(clientconfig.ClientOpts) | ||
| var opts *gophercloud.AuthOptions |
There was a problem hiding this comment.
This seems redundant, as there is opts, err := clientconfig.AuthOptions(clientOpts) below
| return "", fmt.Errorf("No namespace provided, cannot get cacert from configmap") | ||
| } | ||
| if configmapName == "" { | ||
| return "", fmt.Errorf("No name provided, cannot get cacert from configmap") |
There was a problem hiding this comment.
Just a nit about the error messages.
This is the error check that happens right after the call to this function:
if err != nil {
return nil, fmt.Errorf("Failed to get openstack CA cert: %v", err)
}Hence the complete error will be:
Failed to get openstack CA cert: No name provided, cannot get cacert from configmap
You are stating the intent twice.
Usually, you would state the intent in the calling function, because it has inherently more context about what is going on, and why. The name of this function is GetCACertFromConfigmap, but by looking at what it does, it could as well have been GetStringFromConfigmap. The piece of code that knows that the value will be a "CACert" is in the caller only.
This is what you could have for example.
On the caller side:
if err != nil {
return nil, fmt.Errorf("failed to get OpenStack CA certificate from configmap: %v", err)
}and here:
return "", fmt.Errorf("the provided configmap name is an empty string")And for bonus points:
Error strings should not start with a capital letter because they'll often be prefixed before printing
|
|
||
| cacert, err := GetCACertFromConfigmap(kubeClient, "openshift-config", "cloud-provider-config", "ca-bundle.pem") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to get openstack CA cert: %v", err) |
There was a problem hiding this comment.
We don't want to fail if the configmap doesn't contain the cert, it's a valid use case (i.e. non SSL OpenStack)
|
/retest |
1 similar comment
|
/retest |
|
@iamemilio: This pull request references Bugzilla bug 1769879, which is valid. 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 kubernetes/test-infra repository. |
It's a valid use case, for instance when deploying on an OpenStack cloud that doesn't use SSL.
iamemilio
force-pushed
the
ssc-refractor
branch
from
9928bcb to
8f399b2
Compare
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamemilio, mandre 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 |
6 participants
What this PR does / why we need it:
Corrects Self Signed Certs support to pull from centralized location in the cluster, the openshift-config:cloud-provider-config configmap.