buildpacks / libcnb

A non-opinionated language binding for the Cloud Native Buildpack Buildpack and Extension specifications
Apache License 2.0
32 stars 13 forks source link

Distinguish provider and type when reading binding from VCAP_SERVICES #235

Closed c0d1ngm0nk3y closed 1 year ago

c0d1ngm0nk3y commented 1 year ago

see this comment.

When reading bindings from VCAP_SERVICES, provider and type are set and could have different values.

dmikusa commented 1 year ago

This looks good. One question.

I think we should also try to flatten out the credentials into key/value pairs on the binding.

Thoughts on this?

This PR has creds defined as Credentials map[string]string, and the entire Secret value is set to the map. This would make fetching values out of the credentials a little harder because you'd have to look for a binding of the right type, then pull the Credentials, then pull out the value from the map. I think this would also be problematic with auto-config Spring Cloud Bindings which expects bindings with names like url and password, not credentials.

I think we should keep what you have here and put it in as credentials, but we should also flatten the credentials so that there is a new (credentials key as the name and credentials value as the secret) Secret for each key in the Credentials map. That would mean you could look them up either way, fetch the whole set of credentials from one binding or fetch them individually.

My only concern was with overlap in credentials (like if something inside credentials had a key named credentials) and something outside of credentials (like type or credentials), but I don't think that should generally happen.

dmikusa commented 1 year ago

And thanks for submitting this so quickly!

c0d1ngm0nk3y commented 1 year ago

I think this would also be problematic with auto-config Spring Cloud Bindings which expects bindings with names like url and password, not credentials.

I am afraid, I didn't get it yet. Are you referring to this? Since Credentials get passed into Secret (see here), the usage should be the same.

Or what do you mean?

dmikusa commented 1 year ago

I am afraid, I didn't get it yet. Are you referring to this?

Yes.

Since Credentials get passed into Secret (see here), the usage should be the same.

Sorry. That was my misunderstanding. You're right.