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

Patricks blob dir perm #1

Closed
wants to merge 10 commits into from
Closed

Patricks blob dir perm #1

wants to merge 10 commits into from

Conversation

jimfulton
Copy link
Member

This makes it easier to share blob cache dirs, for example when you want to access the database as a normal user and share the cache with a app server process running as a special user.

@jimfulton
Copy link
Member Author

This introduces a backward incompatibility. ZODB.tests.testblob.storage_reusable_suite now requires a factory with a different signature. This means unmodified blob storage implementations will have their tests broken.

I'm guessing that this would effect relstorage and neo, although I'm not positive.

To be safe, we should:

  • Move the tests for permission setting into a separate test that isn't included in storage_reusable_suite, and
  • Add storage_reusable_suite2 that has the new factory signature that includes the tests provided by storage_reusable_suite as well as the permission test.

Also, a point of style. Please don't do:

some_long_function_name(foo,
    bar)

Instead, do:

some_long_function_name(foo,
                        bar)

or:

some_long_function_name(
    foo, bar)

@vpelletier
Copy link
Contributor

This incompatibility won't be a problem for neo because it still lacks blob support.
Jim: Thanks a lot for notifying me of the change.

@tseaver
Copy link
Member

tseaver commented May 20, 2015

@hathawsh Can you figure out if / how Relstorage would be affected?

@jamadden
Copy link
Member

jamadden commented Apr 8, 2017

@jimfulton @tseaver I don't think RelStorage would be affected. It doesn't use ZODB's storage_reusable_suite as far as I can see, it has its own copy. It also doesn't directly use ZODB.blob.BlobStorage.

@jimfulton
Copy link
Member Author

Thanks. (I was going to figure this out myself, now that I'm practically a RelStorage expert. ;))

I'll freshen this PR soon (unless someone beats me to it, of course).

@jimfulton
Copy link
Member Author

Well, the merge with master didn't go well. Still lots of rot.

@jimfulton
Copy link
Member Author

Stepping back

This change was pretty conservative. It kept existing behaviour and provided a way to override it. This was nobley backward compatible.

However, I think everyone (who pays attention) agrees that the existing behavior is stupid. I propose we simply change the behavior to not specify permissions when creating blob files and directories and let users control this the way they control permissions of other files (e.g. umask)

@jimfulton
Copy link
Member Author

Closing in favor of #156

@jimfulton jimfulton closed this Apr 8, 2017
@jimfulton jimfulton deleted the patricks-blob-dir-perm branch May 17, 2017 13:31
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

5 participants