search

cloudfoundry-community / node-cfenv

easy access to your Cloud Foundry application environment for node
Apache License 2.0
73 stars 20 forks source link

add getServiceCredsByLabel #16

Open srl295 opened 8 years ago

srl295 commented 8 years ago

getServiceCredsByLabel is like getServiceCreds except that the label is used.

See discussion https://github.com/cloudfoundry-community/node-cfenv/issues/3

srl295 commented 8 years ago

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

mathieug commented 8 years ago

Cool, thanks :+1: 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 commented 8 years ago

Ok, I've read #3 :)

srl295 commented 8 years ago

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 commented 8 years ago

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.

srl295 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

pmuellr commented 7 years ago

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 commented 7 years ago

Because I am a service provider trying to provide an sdk- specifically, https://github.com/cloudfoundry-community/node-cfenv/issues/3#issuecomment-166983703