Repository navigation
Change ScpCf modules behaviour with DwC bindings - #104
Conversation
…houldThrowForDwcServiceBindings tests
| if( credentials.containsKey("tenantid") ) { | ||
| return credentials.getString("tenantid"); |
There was a problem hiding this comment.
How about extracting this hard-coded string (tenantid) to some proper constant?
(Maybe within the MegacliteServiceBinding class?)
There was a problem hiding this comment.
AFAIK tenantId is used for other platforms as well.
tenantId would ne declared high up the hierarchy.
There was a problem hiding this comment.
I would like to but that would require adding dwc dependencies (MegacliteServiceBinding) in scp-cf modules (ScpCfDestinationServiceAdapter) or the other way around.
| // DwC: The binding doesn't have credentials, MegacliteServiceBinding either reads the DWC_APPLICATION env var | ||
| // and returns the map {tenantid: <ProviderTenantId>}, or, returns an empty map if DWC_APPLICATION is not found. |
There was a problem hiding this comment.
[question]
Has there been a decision whether we keep our old Service Binding (aka Map<String, JsonObject>) API?
Or are these changes here just to make the transition smoother?
There was a problem hiding this comment.
Matthias mentioned he wanted to delete/redo this API
There was a problem hiding this comment.
Sounds good 👍
I'm still wondering about this comment... It doesn't make a lot of sense to me. Why are we mentioning the DwC case here explicitly?
There was a problem hiding this comment.
I'm just helping the next developer that will maintain this code, I can remove it.
There was a problem hiding this comment.
Personally, I find it more confusing than it helps... But maybe I'm the only one 😅
| final TypedMapView credentials = Try.of(() -> { | ||
| final ServiceBinding binding = serviceBindingSupplier.get(); | ||
| return TypedMapView.ofCredentials(binding); | ||
| }) | ||
| .getOrElseThrow( | ||
| e -> new DestinationAccessException( | ||
| "Could not resolve destination to Destination Service on behalf of provider.", | ||
| e)); |
There was a problem hiding this comment.
[minor]
Formatting is pretty odd IMO.
How about:
| final TypedMapView credentials = Try.of(() -> { | |
| final ServiceBinding binding = serviceBindingSupplier.get(); | |
| return TypedMapView.ofCredentials(binding); | |
| }) | |
| .getOrElseThrow( | |
| e -> new DestinationAccessException( | |
| "Could not resolve destination to Destination Service on behalf of provider.", | |
| e)); | |
| final Try<ServiceBinding> maybeServiceBinding = Try.ofSupplier(serviceBindingSupplier); | |
| if( maybeServiceBinding.isFailure() ) { | |
| final String msg = "Could not resolve destination to Destination Service on behalf of provider."; | |
| throw new DestinationAccessException(msg, maybeServiceBinding.getCause()); | |
| } | |
| final TypedMapView credentials = TypedMapView.ofCredentials(maybeServiceBinding.get()); |
There was a problem hiding this comment.
TypedMapView.ofCredentials can throw many Exceptions, I thought it was safer this way.
There was a problem hiding this comment.
TypedMapView.ofCredentialscan throw many Exceptions
True... didn't consider that.
We could change the code to something like that:
| final TypedMapView credentials = Try.of(() -> { | |
| final ServiceBinding binding = serviceBindingSupplier.get(); | |
| return TypedMapView.ofCredentials(binding); | |
| }) | |
| .getOrElseThrow( | |
| e -> new DestinationAccessException( | |
| "Could not resolve destination to Destination Service on behalf of provider.", | |
| e)); | |
| final Try<ServiceBinding> maybeServiceBinding = Try.ofSupplier(serviceBindingSupplier); | |
| if( maybeServiceBinding.isFailure() ) { | |
| final String msg = "Could not resolve destination to Destination Service on behalf of provider."; | |
| throw new DestinationAccessException(msg, maybeServiceBinding.getCause()); | |
| } | |
| final Try<TypedMapView> maybeCredentials = Try.of(() -> TypedMapView.ofCredentials(maybeServiceBinding.get())); | |
| if (maybeCredentials.isFailure()) { | |
| final String msg = "Could not extract credentials from destination to Destination Service."; | |
| throw new DestinationAccessException(msg, maybeCredentials.getCause()); | |
| } | |
| final TypedMapView credentials = maybeCredentials.get(); |
But I'm not sure whether it makes it more readable (maybe a tiny bit).
Context
DwC apps cannot call
ScpCfCloudPlatformwithout theDWC_APPLICATIONenv var. This was introduced in #98.Solution
Here is the intended stacktrace from top to bottom
DwcConfigurationthrowsMegacliteServiceBindingcatches and returns an empty mapScpCfCloudPlatformworks normallyScpCfDestinationServiceAdapterthrows if the map is empty.Questions for the Reviewer
ScpCfDestinationServiceAdapterbe?"No DWC_APPLICATION environment variable defined. Cannot determine the provider account id.". Not sure if there is a scenario in which the Adapter doesn't findtenantidwhile the binding is not DwCDwcConfigurationto be public to help customer test their app locally without setting an env var?MegacliteServiceBinding.getCredentials()cause problem for customers with broken configs?MegacliteServiceBindingpreviously returned an empty map. Also a meaningful DwC error should be thrown in another operation beforehand.