Updating methods in DefaultApplications to accommodate V3 specific implementation #1173
Conversation
abhishek7-sap
changed the title
|
Hello @dmikusa, @pivotal-david-osullivan Please review this Pull Request as a part of lacking v3 operations. |
| .flatMap(function((cloudFoundryClient, spaceId) -> Mono.zip( | ||
| Mono.just(cloudFoundryClient), | ||
| getApplicationIdWhere(cloudFoundryClient, request.getName(), spaceId, sshEnabled(true)) | ||
| getApplicationIdV3(cloudFoundryClient, request.getName(), spaceId) |
There was a problem hiding this comment.
You might consider extracting this into a method. It seems to be common across a few methods. Here, enableSsh and sshEnabled and possibly others. It's minor, but could be a chance to improve the readability of the code, like .flatMap(getApplicationId) instead.
There was a problem hiding this comment.
Hi @dmikusa,
Thank you for reviewing the PR.
I checked your comment. Yes, the code is there multiple times. I believe that even if create a method like Mono<Tuple2<CloudFoundryClient,String>> getCloudFoundryClientAndApplicationIdV3, it will have a similar readability experience.
Please let me know if you still want me to update the method.
There was a problem hiding this comment.
Aside from that one comment, it looks fine. I don't have environments to run the integration tests though. I don't see any changes to the integration tests, which I think is a good sign that this isn't breaking any public APIs.
My only concern is around switching from v2 to v3 in this library. It will force users to switch, and it essentially saying that this is an implementation detail. I am OK with that, but I'd like to get more feedback from folks about their expectations. Is that reasonable to treat the underlying API that operations uses as an implementation detail or should we handle it in some other way (if the latter, how would you all see that working?).
|
Hello @dmikusa
For the integration test, we have created one environment at our labs, And we have specifically tested the integration test of the methods which we have modified. They are green. For running the complete set of integration test, 10% of our test case is failing due to some extra parameter in json response, for which we have raised an issue #1160 (comment). It will be helpful for us, if the tests can be run in one of your working labs or help us in fixing the json response issue.
We have discussed this concern #1160 (comment), I agree with your approach of taking the feedbacks from more folks in community. Please create a platform so that we can discuss on the same. Thanks, Suyash |
|
The branch cannot be rebased because of conflicts. Will you resolve these conflicts and rebase off of |
Updated following methods in DefaultApplications to accommodate V3 specific implementation.
1.)sshEnabled
2.)enableSsh
3.)disableSsh
4.)getEnvironments