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 temp key command #47

Merged
merged 3 commits into from
May 8, 2020
Merged

Fix temp key command #47

merged 3 commits into from
May 8, 2020

Conversation

wfeller
Copy link
Contributor

@wfeller wfeller commented May 7, 2020

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

@iksaku
Copy link
Contributor

iksaku commented May 8, 2020

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.

@iksaku iksaku mentioned this pull request May 8, 2020
2 tasks
@wfeller
Copy link
Contributor Author

wfeller commented May 8, 2020

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
Copy link
Owner

sausin commented May 8, 2020

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 👍

@wfeller
Copy link
Contributor Author

wfeller commented May 8, 2020

Done 👍

@sausin sausin merged commit ec0b345 into sausin:master May 8, 2020
@sausin
Copy link
Owner

sausin commented May 9, 2020

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

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.

None yet

3 participants