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.

Sign up for GitHub

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

Jump to bottom

fixes #1062 : removed empty aws client credentials and set default region #1063

Closed
wants to merge 1 commit into from
Closed

fixes #1062 : removed empty aws client credentials and set default region #1063

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/adapter/etl-adapter-filesystem/src/Flow/ETL/Adapter/Filesystem/AwsS3Stream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,7 @@ protected function filesystem() : Filesystem
*/
$clientOptions = \array_merge(
[
'credentials' => [
'key' => '',
'secret' => '',
],
'region' => '',
'region' => 'us-east-1',
Copy link
Member

Choose a reason for hiding this comment

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

I'm honestly not sure about default region, I think I would rather prefer to throw an exception when the region is not set. Is there anything that makes us-east-1 special enough to make it default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just because it's the default region AWS applies.
IMHO I'd rather avoid merging any default configuration :)

Copy link
Member

Choose a reason for hiding this comment

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

IMHO I'd rather avoid merging any default configuration :)

Same, can't say out of my head why it was implemented this way, I need to take another look later in the codebase but as far as I remember there was a reason. I will look into this later and get back to you

Copy link
Contributor Author

@jguittard jguittard Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Simple : S3 client needs an array with at least region and version set. If $contextOptions does not provide it, it will throw an exception within aws-sdk. Maybe check those 2 keys right before instantiating Filesystem and throw a domain exception instead?

Copy link
Member

Choose a reason for hiding this comment

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

I was playing with it a bit and I think we can skip any defaults, when region or credentials are missing, S3 client will throw an exception which I think is good since it's S3 responsibility, not flow.
Could you remove those merges from both, S3 and Azure clients and in the meantime I'm gonna prepare some examples for the docs that would explain how to use them both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it. I'll take care of it tomorrow

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, examples should be available any moment: #1065

Copy link
Member

Choose a reason for hiding this comment

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

@jguittard would you mind if I would take it over from here and adjust your PR so it can be merged?

'version' => 'latest',
],
$contextOptions[self::PROTOCOL]['client'] ?? []
Expand Down
Loading