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

add getServiceCredsByLabel #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

srl295
Copy link

@srl295 srl295 commented Apr 11, 2016

getServiceCredsByLabel is like getServiceCreds except that the label is used.

  • internal function getServiceByLabel added
  • docs update
  • tests update

See discussion #3

@srl295
Copy link
Author

srl295 commented Apr 11, 2016

Note, other get*ByLabel functions may be desired. This is just a starting point. cc @jthomas

@mathieug
Copy link
Contributor

Cool, thanks 👍
Just a question, why do you need this? I use appEnv.getServiceCreds() for all my user provided services and it also works for my only service which has the same name as its label.

@mathieug
Copy link
Contributor

Ok, I've read #3 :)

@srl295
Copy link
Author

srl295 commented May 16, 2016

okay… so somehow I missed that getAppEnv().services is actually a map keyed by label . docs aren't quite right here.

Upshot is that you can use .services.someService[0] to get the first service labeled someService.

So, there's a workaround here. Not a great workaround, but a workaround.

@pmuellr
Copy link
Member

pmuellr commented May 17, 2016

Not sure what's wrong with the docs; getAppEnv().services is set to the same wonky array/map thing as VCAP_SERVICES, for folks who want that. Docs don't seem to indicate otherwise, to me.

The intent was to be able to provide the original VCAP_SERVICES entry, parsed, for someone's convenience.

If folks are really dead-set on accessing services by label, which seems like a not great idea to me, then really you should add some new functions as this issue suggests, rather than expecting people to access the array/map thing, which is more brittle than a function accessor.

getServiceCredsByLabel is like getServiceCreds except that the label is used.
* internal function getServiceByLabel added
* docs update
* tests update

Note, other get*ByLabel functions may be desired. This is just a starting point.

See discussion cloudfoundry-community#3
@srl295
Copy link
Author

srl295 commented Feb 2, 2017

@pmuellr OK. I've rebased this PR, which adds the function getServiceCredsByLabel, for example:

appEnv.getServiceCredsByLabel(/service.*/) or appEnv.getServiceCredsByLabel("service-a")

tests included. Let me know if you think this is reasonable (even if it needs some caveats about labels).

As a service developer, this API lets our SDK very simply find the service regardless of what the user named it.

@pmuellr
Copy link
Member

pmuellr commented Feb 3, 2017

I'm still confused as to why you wouldn't want getServiceByLabel() and getServiceURLByLabel() (icky casing there) as well. In fact, you implemented getServiceByLabel() but didn't document it! I think if it's going to be exposed, it should be documented (and tested) - lest folks find it and depend on it.

Or maybe no one ever uses anything other than getServiceCreds()?

@srl295
Copy link
Author

srl295 commented Feb 3, 2017

I think it's reasonable to complete the set here. So how about I:

  • document getServiceByLabel()
  • add+document getServiceURLByLabel()

@pmuellr
Copy link
Member

pmuellr commented Feb 3, 2017

Sounds good!

I wonder if we should a note in the docs about usage of these.

The reason I never provided this function is that I've had service providers change the label on the service over time, breaking code. Users have absolutely no control over the label; but they have complete control over the name.

I assume there must be some "pros" to using labels vs names?

@srl295
Copy link
Author

srl295 commented Feb 3, 2017 via email

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

Successfully merging this pull request may close these issues.

3 participants