sausin / laravel-ovh

Wrapper for OVH Object Storage integration with laravel
MIT License
37 stars 12 forks source link

Fix temp key command #47

Closed wfeller closed 4 years ago

wfeller commented 4 years ago

Add option to choose the disk on which to run the command

iksaku commented 4 years ago

I don't think this is the best approach for selecting another OVH Connection Instance...

Maybe, we should implement a native OVH Connection switcher in the core, then add the ability to select different disks in the command.

In the main time, we could re-use part of this PR instead to specify a different container, which should be available for the default connection.

wfeller commented 4 years ago

Actually, in my use case, I have 2 OVH containers, each defined using its own config in my filesystems.disks config and using the ovh driver provided by this package.

The problem now, is that none of my OVH disks is called "ovh" in my filesystems config file, so when the SetTempUrlKey command is registered and constructed, it crashes my app 😅:

Storage::disk('ovh')

Let me know what you think.

sausin commented 4 years ago

Thank you for the pull request!

I am of the opinion that supporting a separate disk has a use case for when the authentication details are different.

That said, what @iksaku mentioned is something that may also be needed. I actually have two containers on a project (one private and one public) which are authenticated by same credentials.

That said, I think the refractor regarding passing the container to the methods doesn't cover the direct goal of supporting a configurable disk. @wfeller If you could please change the submission to remove that, I'm happy to proceed with the PR :+1:

wfeller commented 4 years ago

Done 👍

sausin commented 4 years ago

Will tag a release soon, pending some discussions in #48