Use the Kubernetes API for namespaces check #552
Conversation
We don't need to verify that a namespace exists because we now use a transient namespace and the client will error out if you create a serviceinstace in a namespace that doesn't exist.
openshift-ci-robot
added
the
size/S
|
NACK, it is assumed that the project is created by the catalog. I think this is a valid check IMO. This is probably a pretty good place to determine if that assumption makes sense. |
|
Changes Unknown when pulling 7b2336f on rthallisey:remove-oc-project into ** on openshift:master**. |
|
You can't create a service instance without a valid namespace/project. I don't think there's a scenario where this code will be false. Let me know if you have one in mind. |
|
When someone hits the endpoint without going through the catalog. We are
just a spec conforming thing, we should not assume our callers are doing
the right thing IMO.
…On Tue, Nov 14, 2017 at 1:39 PM Ryan Hallisey ***@***.***> wrote:
You can't create a service instance without a valid namespace/project. I
don't think there's a scenario where this code will be false. Let me know
if you have one in mind.
***@***.*** ansible-service-broker]$ oc create -f scripts/broker-ci/mediawiki123.yaml
Error from server (NotFound): error when creating "scripts/broker-ci/mediawiki123.yaml": namespaces "non-existent-namespace" not found
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#552 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABza92G7M1P1afRbQQ1KZzNTIGfFIVaDks5s2d53gaJpZM4QdrH_>
.
|
There was a problem hiding this comment.
I agree with @shawn-hurley on this one. Instead of removing the check entirely, maybe update it to use the k8s api instead of kubectl?
|
I think this check came from the asbcli era which used the scenario you mentioned. There isn't any harm in keeping another check around, but I think the scenario where this check fails is going to be rare. I have the patch to convert to a namespace already around. I'll swap to that one. |
rthallisey
changed the title
|
Changes Unknown when pulling fb27aa4 on rthallisey:remove-oc-project into ** on openshift:master**. |
pkg/apb/provision_or_update.go
Outdated
| log.Info("Checking if namespace %s exists.", ns) | ||
| _, err = k8scli.CoreV1().Namespaces().Get(ns, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", nil, errors.New(fmt.Sprintf("Project %s does NOT exist!", ns)) |
There was a problem hiding this comment.
=================================
Lint
=================================
pkg/apb/provision_or_update.go:65:19: should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)
Found 1 lint suggestions; failing.
Makefile:27: recipe for target 'lint' failed
make: *** [lint] Error 1
|
Changes Unknown when pulling 5acde6c on rthallisey:remove-oc-project into ** on openshift:master**. |
pkg/apb/provision_or_update.go
Outdated
| log.Info("Checking if namespace %s exists.", ns) | ||
| _, err = k8scli.CoreV1().Namespaces().Get(ns, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return "", nil, fmt.Errorf("Project %s does NOT exist!", ns) |
There was a problem hiding this comment.
One more 😉
pkg/apb/provision_or_update.go:65:30: error strings should not be capitalized or end with punctuation or a newline
You can use make check to run through most of the tests before pushing.
There was a problem hiding this comment.
I'll move back to what was there :).
rthallisey
force-pushed
the
remove-oc-project
branch
from
5acde6c
to
3c97560
Compare
|
thanks @djzager |
|
Changes Unknown when pulling 3c97560 on rthallisey:remove-oc-project into ** on openshift:master**. |
* Remove namespaces check We don't need to verify that a namespace exists because we now use a transient namespace and the client will error out if you create a serviceinstace in a namespace that doesn't exist. * Use the namespaces API * Fix lint test
Describe what this PR does and why we need it:
Verify that a namespace exists using the kubernetes API.
Changes proposed in this pull request