-
Notifications
You must be signed in to change notification settings - Fork 436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify platform behavior when dashboard url is not returned #527
Clarify platform behavior when dashboard url is not returned #527
Conversation
Hey tinygrasshopper! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
spec.md
Outdated
@@ -1086,7 +1086,7 @@ For success responses, the following fields are defined: | |||
|
|||
| Response Field | Type | Description | | |||
| --- | --- | --- | | |||
| dashboard_url | string | The updated URL of a web-based management user interface for the Service Instance; we refer to this as a service dashboard. The URL MUST contain enough information for the dashboard to identify the resource being accessed (`0129d920a083838` in the example below). Note: a Service Broker that wishes to return `dashboard_url` for a Service Instance MUST return it with the initial response to the update request, even if the service is updated asynchronously. If present, MUST be a non-empty string. | | |||
| dashboard_url | string | The updated URL of a web-based management user interface for the Service Instance; we refer to this as a service dashboard. The URL MUST contain enough information for the dashboard to identify the resource being accessed (`0129d920a083838` in the example below). Note: a Service Broker that wishes to return `dashboard_url` for a Service Instance MUST return it with the initial response to the update request, even if the service is updated asynchronously. If present, MUST be a non-empty string. If not present, the Platform MUST retain the previous value of the `dashboard_url`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/not/NOT
else travis will complain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might as well call out that updating to an unset value is not currently allowed by the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @tinygrasshopper is working on a way to deal with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattmcneeney which not
are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The not in If not present
. Would travis not shout about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's not an RFC2119 keyword by itself. If its next to "MUST" then it needs to be in caps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We thought about the unset dashboard url case, all the solutions we came up for the usecase had a high cost of change in the spec. I am tending toward not solving it right now, as we dont have any known usecases of the removal flow right now. With this change I would like to clarify the existing behaviour.
@pmorie I am in two minds about calling out unsupported flows in the spec, as there is no precedence for that in the rest of the spec.
One minor nit - there's a misspelling in the commit message ('clarify') |
fb92979
to
7e69b11
Compare
rebase needed |
SGTM just waiting for the rebase |
@tinygrasshopper We discussed this on today's call and are happy to start reviewing, but it needs a rebase first! |
7e69b11
to
cd39b3d
Compare
@mattmcneeney rebased |
while polling for updates
while polling on update.
Have not expressed an opinion on how the broker can clear out an existing dashboard url, but clarified current behaviour in a way that its backwards compatible.