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

endpoint configuration for s3 storage #1774

Merged
merged 1 commit into from
Aug 6, 2020
Merged

endpoint configuration for s3 storage #1774

merged 1 commit into from
Aug 6, 2020

Conversation

michalpicpauer
Copy link
Contributor

@michalpicpauer michalpicpauer commented Jul 30, 2020

Subject

Add endpoint configuration option for s3 filesystem.

I am targeting this branch, because these changes respect BC.

Closes #1773 #1706

Changelog

### Added
- Added configuration option filesystem.s3.endpoint

Copy link
Member

@core23 core23 left a comment

Choose a reason for hiding this comment

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

Can you please provide a test, so we don't get unexpected exceptions when not defining the key

@michalpicpauer
Copy link
Contributor Author

Ok, I made some changes. Is it fine to update the existing test with this new key?

@anacona16 anacona16 mentioned this pull request Jul 30, 2020
2 tasks
@jordisala1991
Copy link
Member

Yes please, update the tests and add a new one to test what happens when this new key is configured

@jordisala1991
Copy link
Member

Plus it would be nice to update docs with that addition

@michalpicpauer
Copy link
Contributor Author

Yes please, update the tests and add a new one to test what happens when this new key is configured

The updated test should cover both situations or am I missing something?

jordisala1991
jordisala1991 previously approved these changes Jul 31, 2020
@@ -399,6 +399,10 @@ public function configureFilesystemAdapter(ContainerBuilder $container, array $c
'version' => $config['filesystem']['s3']['version'],
];

if (isset($config['filesystem']['s3']['endpoint'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add an other test that does not modify the endpoint and skips this line

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Only half of tests go inside the if

Captura de pantalla 2020-07-31 a las 14 01 30

jordisala1991
jordisala1991 previously approved these changes Aug 1, 2020
Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

Please squash your commits into 1

@VincentLanglet
Copy link
Member

Please review @core23 :)
Does the @jordisala1991 answer is enough for you ?

Copy link
Member

@core23 core23 left a comment

Choose a reason for hiding this comment

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

There is still a test missing that is checking for a null endpoint value, but I don't want to block this PR.

@jordisala1991 jordisala1991 merged commit c2ed47c into sonata-project:3.x Aug 6, 2020
@jordisala1991
Copy link
Member

Thank you @michalpicpauer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for S3 api compatible storages
5 participants