← 返回首页
Updating methods in DefaultApplications to accommodate V3 specific implementation by abhishek7-sap · Pull Request #1173 · cloudfoundry/cf-java-client · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

Updating methods in DefaultApplications to accommodate V3 specific implementation#1173

Closed
abhishek7-sap wants to merge 4 commits into
cloudfoundry:mainfrom
abhishek7-sap:defaultApplications-api
Closed

Updating methods in DefaultApplications to accommodate V3 specific implementation#1173
abhishek7-sap wants to merge 4 commits into
cloudfoundry:mainfrom
abhishek7-sap:defaultApplications-api

Conversation

abhishek7-sap commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Updated following methods in DefaultApplications to accommodate V3 specific implementation.
1.)sshEnabled
2.)enableSsh
3.)disableSsh
4.)getEnvironments

linux-foundation-easycla Bot commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown



The committers listed above are authorized under a signed CLA.

abhishek7-sap changed the title defaultApplication-api Updating methods in DefaultApplications to accommodate V3 specific implementation Jan 4, 2023
suyash-sap mentioned this pull request Jan 13, 2023
Closed

suyash-sap commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

Hello @dmikusa, @pivotal-david-osullivan Please review this Pull Request as a part of lacking v3 operations.

dmikusa reviewed Feb 3, 2023
View reviewed changes
Comment thread ...y-operations/src/main/java/org/cloudfoundry/operations/applications/DefaultApplications.java
Comment on lines 234 to +236
.flatMap(function((cloudFoundryClient, spaceId) -> Mono.zip(
Mono.just(cloudFoundryClient),
getApplicationIdWhere(cloudFoundryClient, request.getName(), spaceId, sshEnabled(true))
getApplicationIdV3(cloudFoundryClient, request.getName(), spaceId)

dmikusa Feb 3, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide 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.

abhishek7-sap Feb 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide 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.

dmikusa reviewed Feb 3, 2023
View reviewed changes

dmikusa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide 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?).

suyash-sap commented Mar 1, 2023

Copy link
Copy Markdown
Contributor

Hello @dmikusa

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.

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.

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?)

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

radoslav-tomov commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

I ran the integration tests against the change and it does seem to brake anything. I have a small number of tests failing, but as far as I can tell, they are unrelated. @dmikusa @mheath Can you check the PR and merge it ?

radoslav-tomov approved these changes Aug 8, 2023
View reviewed changes

radoslav-tomov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

LGTM

stephanme mentioned this pull request Aug 10, 2023
Merged
mheath self-requested a review August 10, 2023 22:15
mheath approved these changes Aug 10, 2023
View reviewed changes

mheath left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

All these change look good to me.

I don't have strong opinions about Daniel's suggestion for moving duplicate code into its own method. There are pros and cons to both.

mheath commented Aug 10, 2023

Copy link
Copy Markdown

The branch cannot be rebased because of conflicts. Will you resolve these conflicts and rebase off of main?

pivotal-david-osullivan mentioned this pull request Feb 13, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dmikusa dmikusa left review comments
pivotal-david-osullivan Awaiting requested review from pivotal-david-osullivan
+2 more reviewers
mheath mheath approved these changes
radoslav-tomov radoslav-tomov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Footer

© 2026 GitHub, Inc.