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

added missed "options" argument for AwsAdapterDefinitionBuilder #18

Closed
wants to merge 2 commits into from

Conversation

psolom
Copy link

@psolom psolom commented Jun 18, 2019

League\Flysystem\AwsS3v3\AwsS3Adapter supports 4th argument named "options", but it's missed in this package.

With the proposed changes I can now describe AWS storage as the following:

flysystem:
    storages:
        aws.storage:
            adapter: 'aws'
            options:
                client: 'aws-client-id'
                bucket: 'bucket-name'
                options:
                    ACL: 'public-read'

You can see the ACL: 'public-read' which is now passed to AwsS3Adapter

Please release new version after merge.

psolom and others added 2 commits June 18, 2019 13:17
League\Flysystem\AwsS3v3\AwsS3Adapter supports 4th argument named "options", but it's missed in this package.

With the proposed changes I can now describe AWS storage as the following:

```
flysystem:
    storages:
        aws.storage:
            adapter: 'aws'
            options:
                client: 'aws-client-id'
                bucket: 'bucket-name'
                options:
                    ACL: 'public-read'
```

You can see the `ACL: 'public-read'` which is now passed to AwsS3Adapter

Please release new version after merge.
added missed "options" argument for AwsAdapterDefinitionBuilder
@tgalopin
Copy link
Member

Hello @servocoder !

Thanks for your PR! Could you rebase on master (to avoid having a duplicated commit in the PR) and add the option in tests (don't hesitate to use #12 as an example :) ).

@psolom
Copy link
Author

psolom commented Jun 18, 2019

Rebase is not the thing I'm experienced a lot with. Perhaps it would be better if you close my PR and commit the changes at your own.

@tgalopin
Copy link
Member

Ah, I understand! We can do it two ways then:

  • I can give you a hand to finish this yourself (and it'll help you do it in the future too ;) ): if so, let's discuss on Symfony Slack (symfony.com/support)
  • Or I can update this PR in order to fix things, if you don't have time to finalize all this :)

@psolom
Copy link
Author

psolom commented Jun 18, 2019

Go with option 2 please - complete it for me
I'm really a lack of time to rework this PR properly
And thanks for your suggestion of assistance, appreciate it!

@psolom
Copy link
Author

psolom commented Jul 1, 2019

Hey @tgalopin, any chance to get it done soon?

@psolom
Copy link
Author

psolom commented Jul 23, 2019

gentle reminder

@tgalopin
Copy link
Member

tgalopin commented Jul 23, 2019

Hello @servocoder!

Thanks for the reminder :) . Unfortunately, I'm very busy this summer and I try to focus my open-source work to what's needed for maintenance and stability. When I'll have time, I'll work on new features such as this one :) .

If someone else wants to create a PR on this, feel free to do so!

EDIT: "closed" misclick

@nico-incubiq
Copy link

@tgalopin @servocoder I opened #22 which is based on latest master and has added test coverage for the new options. hope it helps

@tgalopin
Copy link
Member

Fixed by #22

@tgalopin tgalopin closed this Aug 21, 2019
@tgalopin
Copy link
Member

Thanks @servocoder for the original PR and @nico-incubiq for having finished it. OSS is powerful :) .

@psolom
Copy link
Author

psolom commented Aug 21, 2019

Great! Thanks @nico-incubiq
@tgalopin Looking forward a new package release

@tgalopin
Copy link
Member

I'm waiting a few days to see if #19 can be merged but I'll release a new tag yes :) .

@tgalopin
Copy link
Member

@psolom
Copy link
Author

psolom commented Aug 27, 2019

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants