feat: make endpointOverride configurable by DataAddress #68
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 72.86% 73.19% +0.33%
==========================================
Files 20 20
Lines 479 485 +6
Branches 17 17
==========================================
+ Hits 349 355 +6
Misses 129 129
Partials 1 1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for taking care, I think there are some issues in the designed proposed:
making the AwsClientProvider mutable would be pretty dangerous, it is a component used by potentially different threads, and so it should never change once is instantiated (immutable).
my suggestion would be to overload the s3Client with a version that accept a custom endpointOverride that will eventually being used instead of the one in the configuration.
note: This could be an occasion to introduce a data object called like S3ClientRequest that contains region, secretToken and endpointOverride, so at the end there'll be only one s3Client method with that parameter. not mandatory but definitely a good refactoring work to be done at the end.
…modify configuration, refactoring
|
@ndr-brt please take a look now - is this what you expected? |
...sions/common/aws/aws-s3-core/src/main/java/org/eclipse/edc/aws/s3/AwsClientProviderImpl.java
Outdated
Show resolved
Hide resolved
...sions/common/aws/aws-s3-core/src/main/java/org/eclipse/edc/aws/s3/AwsClientProviderImpl.java
Outdated
Show resolved
Hide resolved
...sions/common/aws/aws-s3-core/src/main/java/org/eclipse/edc/aws/s3/AwsClientProviderImpl.java
Outdated
Show resolved
Hide resolved
extensions/common/aws/aws-s3-core/src/main/java/org/eclipse/edc/aws/s3/S3ClientRequest.java
Outdated
Show resolved
Hide resolved
...ws/aws-s3-test/src/testFixtures/java/org/eclipse/edc/aws/s3/testfixtures/AbstractS3Test.java
Show resolved
Hide resolved
...sions/common/aws/aws-s3-core/src/main/java/org/eclipse/edc/aws/s3/AwsClientProviderImpl.java
Outdated
Show resolved
Hide resolved
…s, tests refactoring
|
@ndr-brt please take a look at newest changes, thanks in advance! |
|
@maciejkizlich-zf just re-request my review through github without adding a comment, so I will be notified correctly ;) |
maciejkizlich-zf
changed the title
…ond minio instance to git workflow
|
thanks! |
What this PR changes/adds
PR adds possibility to specify 'endpointOverride' property in DataAddress.
Why it does that
So that it's possible to push data into different instance of S3 compatible storage than configured via dataplane's properties.
Further notes
Change mostly based on previous PR (eclipse-edc/Connector#1904) by @reisman234, recreated due to changes in project structure.
Linked Issue(s)
Closes #24