Skip to content
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

Types in details structs are different #36

Closed
joek opened this issue Dec 7, 2016 · 4 comments
Closed

Types in details structs are different #36

joek opened this issue Dec 7, 2016 · 4 comments

Comments

@joek
Copy link
Contributor

joek commented Dec 7, 2016

Hi,
as you might have ssen I opend a pull request, which was not done so far. #35

What I like to know is, which direction should we go. Should all the *Details Structs have a RawParameteres field, should the PrivisionDetails struct be changed back to a map type Parameters field or should everything stay as it is ;-)

Cheers
Johannes

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@avade
Copy link
Contributor

avade commented Dec 8, 2016

Thanks for raising the @joek.

The team are going to discuss this soon and we will get back to you with an answer!

@Samze
Copy link
Member

Samze commented Jan 3, 2017

Hey @joek Thanks for submitting the PR.

You covered three changes in #35. Would you mind submitting three separate PRs for each? It's just a bit cleaner.

  1. Regarding the RawParameters inconsistency, we agree that it would be best if the BindDetails/UpdateDetails had a RawParameters field with type json.RawMessage.
  2. With regards to the new errors you introduced in Some cleanup #35, what's the point of having them if the broker api code doesn't check for them (i.e. https://github.com/pivotal-cf/brokerapi/blob/master/api.go#L102 for update and https://github.com/pivotal-cf/brokerapi/blob/master/api.go#L255 for bind)? What do you think these errors should translate to in HTTP codes? It does not seem that the broker spec accounts for them at the moment.
  3. Your changes regarding LastOperationState in LastOperationResponse look good.

And yes, please run (and fix) the tests. 😻

-- @Samze & @st3v

@joek
Copy link
Contributor Author

joek commented Jan 25, 2017

@Samze thanks for your response. I will send pull requests as soon as I cleaned up my messy changes and added testing ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants