send cluster ID as the organization GUID #2103
Conversation
k8s-ci-robot
added
size/S
|
two types of failures:
|
|
2 is a total violation of the spec, so all of those tests should be failing anyway. |
|
/close I don't have time to fix this all the way down the stack. filed kubernetes-retired/go-open-service-broker-client#121 to fix it in the client. |
|
I'll reopen it when the upstream issue is fixed if someone hasn't
already taken it.
…On 06/14/2018 10:14 AM, Morgan Bauer wrote:
/close I don't have time to fix this all the way down the stack. filed
kubernetes-retired/go-open-service-broker-client#121
<kubernetes-retired/go-open-service-broker-client#121>
to fix it in the client.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2103 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMuDAW_OuElsWI4-L1N0L5O_6hyNnasOks5t8poTgaJpZM4Ug7Fb>.
|
k8s-ci-robot
added
size/M
|
fixed and green. todos left for future tests to write and upstream clients to fix. good enough to close #800 |
| Context: testContext, | ||
| Parameters: tc.expectedParams, | ||
| OrganizationGUID: testClusterID, | ||
| // TODO SpaceGUID: testNamespaceGUID, |
There was a problem hiding this comment.
What's up with the lingering TODO? If it needs to stick around, let's provide more info in the comment on what it's waiting for.
Farther down it's being set so I'm not sure why these can't be set too, but I really don't know what SpaceGUID is. 😀
There was a problem hiding this comment.
@MHBauer any reason we're not just getting it to the Kube namespace guid? In CF, "Space" is similar to a Kube "namespace" - just a grouping mechanism for apps. Typically, people have a space for dev, test and prod.
There was a problem hiding this comment.
It doesn't look like there's any interesting setup required to make SpaceGUID work?
If we removed the comments and started setting SpaceGUID: testNamespaceGUID here just like we already do further below, it seems like it would just work (I haven't tried that). Since we are kind of touching this code (i.e. this PR is introducing TODOs), if it doesn't cause test failures, I'd prefer to fix these while we are in here.
There was a problem hiding this comment.
Every test with a TODO fails.
|
get namespace was never set up in any of the tests. Thus, out of scope for the issue. Apparently the fake thinks |
|
Going to be captive on a plane for a couple hours. I'll take a look at the TODOs. Otherwise, if I made sure to fix every problem I saw while solving another problem, I'd never finish anything. This is a completed unit of work as is. We can file an issue to follow up on the TODOs. |
My point was only that since your PR added the todos we should either fix them here, or comment them better so that someone else knows what should happen later. 😀 I am fine with a follow-up |
|
issue #2151 created as followup |
|
okay, maybe done. |
|
Seems good. |
cleaned up the imports as the gofmt tool wanted to move entries around
- default test namespace - prepend a get namesapce failure in namespace failure tests
|
rebased |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing 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 |
|
updated to address newly added tests. |
|
LGTM |
|
This still seems good to me. |
cleaned up the imports as the gofmt tool wanted to move entries around
resolves #800
/cc @arschles @duglin