-
Notifications
You must be signed in to change notification settings - Fork 20
Transparent Proxy: Seamless handling of Cross-level consumption of destination-fragment pairs #903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
32591b7
to
c2a9936
Compare
c2a9936
to
05d9efa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also update the PR description and sign the CLA 😉
@@ -673,6 +675,34 @@ public DynamicBuilder fragmentName( @Nonnull final String fragmentName ) | |||
return header(new Header(FRAGMENT_NAME_HEADER_KEY, fragmentName)); | |||
} | |||
|
|||
/** | |||
* Sets the destination level for the dynamic destination. See | |||
* https://help.sap.com/docs/connectivity/sap-btp-connectivity-cf/dynamic-lookup-of-destinations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any docs here about the level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of the documentation with the dynamic lookup of destinations is not yet applied to the public documentation since this feature is not released in the transparent proxy yet. You can see the corresponding page in the internal documentation: https://wiki.one.int.sap/wiki/pages/viewpage.action?pageId=3811223491
However, I have used the public documentation URL since the change will be applied there with the next transparent proxy release.
* @return This builder instance for method chaining. | ||
*/ | ||
@Nonnull | ||
public DynamicBuilder destinationLevel( @Nonnull final String destinationLevel ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is either "subaccount" or "instance". For such cases, we usually use enums and, in fact, just introduced one: https://github.com/SAP/cloud-sdk-java/blob/main/cloudplatform/connectivity-destination-service/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceOptionsAugmenter.java#L139
But I also assume the two From the test code below it looks like provider is also supported, then let's reuse the enum.provider
options don't apply to transparent Proxy? If it's just two options and not 4, then you could also consider to default to subaccount
and just add one method without args to explicitly change it to .instanceLevel()
What is the behavior of TP if nothing is given? I assume subaccount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are four possible values for both destinationLevel and fragmentLevel - "provider_subaccount", "provider_instance", "subaccount" and "instance". The behaviour of the TP if nothing is given matches that of the Destination Service - the "default level" is "instnace ".
I have applied the DestinationServiceOptionsAugmenter.CrossLevelScope enum which fits perfectly for this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "default level" is "instnace ".
for the /v2 consume API yes, for the /v1 find destination it will fallback to subaccount. So I assume TP uses /v2 under the hood?
Because the Cloud SDK behaves according to /v1 by default, and only if cross-level is enabled, it uses /v2...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct. The Transparent proxy works analogically - it uses the /v2 consume API when levels are defined in a either in a destination CR or as headers (the approach used in TP-CloudSDK integration). Otherwise the /v1 API is used by default.
Got it, I now marked this PR as draft and we can merge it once the release is available. @panayotmarinov kindly ping us when the release is ready and the PR has been finalized (merge conflicts, PR description, etc.). Thanks! |
05d9efa
to
aa3d017
Compare
Context
SAP/cloud-sdk-java-backlog#ISSUENUMBER.
Please provide a short description of what your change does and why it is needed.
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated