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

ISSUE-2161 Pass service binding parameters to service broker #2163

Conversation

svkrieger
Copy link
Contributor

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177475107

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

@JenGoldstrich
Copy link
Contributor

Hey @svkrieger, this PR looks good to us, one thing @tstannard and I think you could maybe add explicit unit tests for when you pass in service binding parameters, and when you don't. Otherwise this seems reasonable to us, will ping folks from the services team on slack for them to also scan through this to double check that.

Thanks,
Jenna & Teal

@svkrieger svkrieger force-pushed the ISSUE-2161-pass-service-binding-parameters-to-sb branch 2 times, most recently from 5030a67 to cc3870c Compare March 25, 2021 15:55
@svkrieger svkrieger force-pushed the ISSUE-2161-pass-service-binding-parameters-to-sb branch from cc3870c to 7ed709d Compare March 25, 2021 15:58
@svkrieger
Copy link
Contributor Author

@JenGoldstrich I can't really come up with a meaningful explicit test for the arbitrary parameters, without adding two contexts and duplicating some tests, as it is tested implicitly at the moment. And I don't think it would add that much confidence to be worth it. But let me know if you feel different. Or feel free to add one yourself, if you have one in mind.

@JenGoldstrich
Copy link
Contributor

Merging, see further context: https://cloudfoundry.slack.com/archives/C07C04W4Q/p1616585842025800

Thanks again @svkrieger !!!

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177501189

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

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

Successfully merging this pull request may close these issues.

4 participants