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

Expose Storage pre-signed object delete url #306

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

nolith
Copy link
Contributor

@nolith nolith commented Feb 21, 2018

This PR exposes pre-signed URLs for Google Cloud Storage

@icco
Copy link
Member

icco commented Feb 26, 2018

Hi @nolith! Thanks for the PR. This actually looks like it might be better served by modifying https://github.com/fog/fog-google/blob/master/lib/fog/storage/google_json/requests/get_object_https_url.rb to have an optional argument of method that defaults to "GET", that way we don't need to create a new file for every single request type. What do you think?

@icco icco self-requested a review February 26, 2018 13:02
@nolith
Copy link
Contributor Author

nolith commented Feb 27, 2018

Hi @icco thank you.

Looking at the code base I had the impression that each kind of request is supposed to be on a different class loaded with request.

This is exactly the same way get_object/get_object_url and put_object/put_object_url is implemented.

Adding an optional parameter to get_object_https_url it's a bit strange to me because I had the impression that get_ doesn't mean getter method but it's the name of the remote operation API, which is GetObject

@nolith
Copy link
Contributor Author

nolith commented Mar 2, 2018

The same PR has merged and released by fog-aws.

I would love to remove this monkey-patching from my production code.
Please @icco let me know what can I do to bring this forward. 🙏

Thanks

Copy link
Member

@icco icco left a comment

Choose a reason for hiding this comment

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

Sorry, thought I had responded. Seems reasonable. In general we just like to remind people that the request level APIs are not stable, since this implies you won't be using this via a model api.

Thanks for your contribution!

@icco icco merged commit 8ade854 into fog:master Mar 2, 2018
@nolith
Copy link
Contributor Author

nolith commented Mar 2, 2018

No problem. Thank you

@nolith nolith deleted the delete_object_url branch March 2, 2018 13:32
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.

2 participants