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

Allow passing predefined_acl via gcs blob_properties #727

Closed
wants to merge 1 commit into from

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Sep 28, 2022

Title

Allow passing predefined_acl via gcs blob_properties

Motivation

Similar to #726, it should be possible to set an ACL for gcs uploads, as it is for s3.

It turns out you can't set an ACL to a gcs blob object directly ref stevearc/pypicloud#317.

Tests

If you're fixing a bug, consider test-driven development:

  1. Create a unit test that demonstrates the bug. The test should fail.
  2. Implement your bug fix.
  3. The test you created should now pass.

If you're implementing a new feature, include unit tests for it.

Make sure all existing unit tests pass.
You can run them locally using:

pytest smart_open

If there are any failures, please fix them before creating the PR (or mark it as WIP, see below).

Work in progress

If you're still working on your PR, include "WIP" in the title.
We'll skip reviewing it for the time being.
Once you're ready to review, remove the "WIP" from the title, and ping one of the maintainers (e.g. mpenkov).

Checklist

Before you create the PR, please make sure you have:

  • Picked a concise, informative and complete title
  • Clearly explained the motivation behind the PR
  • Linked to any existing issues that your PR will be solving
  • Included tests for any new functionality
  • Checked that all unit tests pass

Workflow

Please avoid rebasing and force-pushing to the branch of the PR once a review is in progress.
Rebasing can make your commits look a bit cleaner, but it also makes life more difficult from the reviewer, because they are no longer able to distinguish between code that has already been reviewed, and unreviewed code.

smart_open/gcs.py Outdated Show resolved Hide resolved
@mpenkov
Copy link
Collaborator

mpenkov commented Oct 16, 2022

Is it possible to set the ACL after the upload is complete? If we can do this bypassing smart_open, that would be ideal.

@ddelange
Copy link
Contributor Author

ddelange commented Oct 16, 2022

do you mean as a fallback for gcs <= 2.6?
or as primary/only approach?

it was my first thought too. but since I dont use gcs, I had to do a deep dive into the code and have not found a way to set ACL after upload:

there is blob.update(), but there is no attribute setter for ACL.

so using predefined_acl kwarg in one of the upload methods is the only way I could find to achieve blob upload with ACL. all upload methods have had this kwarg for a long time (ACL was introduced in 1.8.0), ONLY the method smart_open uses didn't have it... So my PR there and then this PR here became the path of least resistance towards stevearc/pypicloud#317

@ddelange ddelange mentioned this pull request Oct 16, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Oct 16, 2022

I meant as a primary (only) approach.

This looks relevant: https://cloud.google.com/storage/docs/xml-api/put-object-acls

@ddelange
Copy link
Contributor Author

looks like this PR can be superceded by #729 (comment)

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 17, 2022

OK, thank you. Closing in favor of #729

@mpenkov mpenkov closed this Oct 17, 2022
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