Repository navigation
Refactor: Remove CloudPlatform classes - #111
Conversation
Johannes-Schneider
added
please merge
| private Stream<Map<String, Object>> streamServiceCredentialsByPlan( @Nonnull final String serviceName ) | ||
| { | ||
| return DefaultServiceBindingAccessor | ||
| .getInstance() |
There was a problem hiding this comment.
(Comment/Question)
I'm not sure what is expected best practice for this API:
- invoke
getInstanceeverytime, not storing the accessor, making (local) mocking and future workarounds difficult - invoke
getInstanceupon class instantiation, storing the accessor, allowing for easy (local) mocking and future workarounds
There was a problem hiding this comment.
Good question.
In general, I imagined the getInstance method to be called every time. Otherwise, applications might be overwriting the instance at some point and run into unexpected and hard to debug issues since the instance they set is not actually being used everywhere.
(Consider our regular Accessor classes: We are calling the underlying Facade every time as well)
With regards to mocking / testing:
I share your concern: I don't like the fact that customers / we have to know this implementation detail to effectively mock this class - that is: It is not apparent from any API that we are using this static instance inside the implementation.
However, mocking this statically stored instance is pretty easy and straight forwards (given the fact that you KNOW that it is being used) IMO.
newtork
left a comment
newtork
left a comment
There was a problem hiding this comment.
LGTM
Hopefully we will not regret removing all those test cases. I know it's unused code but now we definitely have to trust security library to handle their test coverage just as well 😁
| @Test | ||
| public void dwcBindingDoesNotThrow() | ||
| { | ||
| final MegacliteServiceBinding serviceBinding = | ||
| MegacliteServiceBinding | ||
| .forService(ServiceIdentifier.DESTINATION) | ||
| .providerConfiguration() | ||
| .name("destination-paas") | ||
| .version("v1") | ||
| .build(); |
There was a problem hiding this comment.
Can this test be migrated to the new service binding?
There was a problem hiding this comment.
What's being tested here was our implementation of converting ServiceBinding instances into the Map<String, JsonObject> format we used to have in our ScpCfCloudPlatform.
Therefore, I don't think we need to migrate this test as we simply no longer offer this functionality.
There was a problem hiding this comment.
MegacliteServiceBinding.getCredentials() returns Map<String, Object> to ScpCfTenantFacadestreamServiceCredentialsByPlan(). This functionality is new.
There was a problem hiding this comment.
I added a corresponding test for the added method.
Co-authored-by: Charles Dubois <103174266+CharlesDuboisSAP@users.noreply.github.com>
Context
BLI: https://github.com/SAP/cloud-sdk-java-backlog/issues/201
This PR removes classes related to the
CloudPlatformfrom the SDK.Feature Scope
CloudPlatformAccessorCloudPlatformFacade(and implementations)CloudPlatform(and implementations)CloudPlatformExceptionMigration guide updated→ will be done in refactor: RemoveServiceBindingLibWorkaroundClass #307