Skip to content
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

fix: Fix for storing all objects in case of 'folder' transfer #156

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

dorkau80
Copy link
Contributor

@dorkau80 dorkau80 commented Dec 4, 2023

What this PR changes/adds

Fix issue of transferring only last object in case of folder transfer
Fix issue with missing complete file per transferred object
Fix error in resolving secret for empty key
Adds integration test for multiobjects transfer

Why it does that

Currently multiple object transfer does not work as expected - it stores all objects with the same name - which means that all apart from last one gets overwritten.

Further notes

Additionally this PR adjust existing Integration test to test single object transfer as well as multi objects transfer.

Linked Issue(s)

I have noticed that similar issue was created today by @Pluci - so I did not double the issue
Closes #154

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

as an another PR about the same issue already existed, please add review on that one.
this can be closed as duplicated #155

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f7a4cba) 64.71% compared to head (be04bfa) 65.06%.
Report is 5 commits behind head on main.

Files Patch % Lines
...pse/edc/connector/dataplane/aws/s3/S3DataSink.java 85.71% 2 Missing and 1 partial ⚠️
...onnector/dataplane/aws/s3/S3DataSourceFactory.java 0.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   64.71%   65.06%   +0.35%     
==========================================
  Files          26       26              
  Lines         632      647      +15     
  Branches       28       30       +2     
==========================================
+ Hits          409      421      +12     
- Misses        218      219       +1     
- Partials        5        7       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paullatzelsperger paullatzelsperger added the enhancement New feature or request label Dec 7, 2023
@dorkau80 dorkau80 requested a review from yurimssilva December 8, 2023 17:03
@dorkau80 dorkau80 requested a review from yurimssilva December 12, 2023 20:37
@dorkau80
Copy link
Contributor Author

@paullatzelsperger / @ndr-brt - could you approve the awaiting workflows?
I hope it should be the last round - it is only about updating the DEPENDENCIES file.

@paullatzelsperger
Copy link
Member

@dorkau80 DEPENDENCIES is outdated again. It's a pain, but one we cannot avoid unfortunately.

Regarding the duplicate PR #155: on first glance it looks as though your PR is more comprehensive, but I haven't looked in detail. Seems like there is some cross-pr-communication going on anyway. Were you able to coordinate with @Pluci about possibly merging those two PRs?

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

for me merging this or the other does not make much difference, but please have some alignment so you decide which PR will make it

@dorkau80
Copy link
Contributor Author

@Pluci - Is it ok for you if we go with my PR? I think I have addressed all the comments from your and my code review.

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Once the conflicts with the main branch are solved we can merge this, the PR provided by @Pluci is stale so I'd prefer this one

@dorkau80
Copy link
Contributor Author

@ndr-brt - conflicts solved.

@ndr-brt ndr-brt merged commit 421bdc0 into eclipse-edc:main Dec 20, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transfer Multiple S3 Objects not working as expected
5 participants