search

Squarespace / pgbedrock

Manage a Postgres cluster's roles, role memberships, schema ownership, and privileges
https://pgbedrock.readthedocs.io/en/latest/
Other
311 stars 35 forks source link

WIP added is_google_cloud to version info for issue #12 #40

Closed zamai closed 5 years ago

zamai commented 6 years ago

Hey @zcmarine,

I've added support for different roles_table in the context.py SQL queries. Initially, I wanted to have them as parameters that would be passed to currsor.execute() - but you can not pass the table name to that method, so I opted for string formatting.

With my local postgres and remote googel postgres:

Would like to know if this direction makes sence to you

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 161

Totals Coverage Status
Change from base Build 158: 0.0%
Covered Lines:
Relevant Lines: 0
💛 - Coveralls
zcmarine commented 6 years ago

Hey @zamai, sorry for the delay in responding. My last day at Squarespace was on Monday and I'm heading out traveling for 2 1/2 months starting Saturday, so I've been kind of busy. Once I'm traveling I'll try to check in, but I won't have a laptop and will be traveling in eastern Africa so I'll be depending on internet cafes and whatever cadence I can get to them. Just wanted to give you a heads-up on that! There are a few other people at Squarespace that are starting to get involved in this project and may be able to help in bouncing ideas around / offering guidance on development. Of them, @sturzhav and @cpdean are the most knowledgeable.

This looks like a reasonable direction to go down. In the long-term, I think we'll end up extracting out the queries for each implementation (Postgres, Redshift, etc.), but until we reach that point it would be premature optimization so I like your simple modifications of the existing queries for the moment.

I wouldn't feel comfortable merging until we can get all submodules within pgbedrock generate and pgbedrock configure working, and we get tests around the Google SQL stuff, but this seems like a great start! My reasoning here is that I don't want to have a fundamentally different contract with end users across implementations, as that is both confusing for us to explain, confusing for them to parse, and confusing for us to maintain. Under the To Dos in Issue #12 I had a few thoughts on how to accomplish the above asks, so hopefully they seem within reach to you. If you have any other ideas on how to accomplish them, I'm happy to talk through them as well (probably in that Issue, to keep it all organized).

So in short, sorry for being slow to respond and this looks like a great start!!

zcmarine commented 5 years ago

Hey @zamai, sorry for the long delay; I'm back in the US now. You said above that pgbedrock ran fine with Google Cloud SQL in both generate and configure modes; is that true if a change to default privileges is needed (e.g. granting read to some_schema.*?). Given what was said in issue #12 in this summary I think that's still gonna be broken for Google Cloud SQL.

I'm fine with merging this PR once the above comments are resolved, but ideally the default privileges would be addressed in a PR as well before we release a new version supporting Google Cloud SQL to the best of our current abilities. What do you think?

On that point, I think it'd be pretty easy to short-circuit default privileges (i.e. just don't do any default privilege management since we don't know how to make it work). It'd really just be a matter of changing the self.default_acl_possible variable defined here, perhaps by passing a variable to PrivilegeAnalyzer that says not to configure default privileges. Again, that could be a separate PR and could even be skipped if you want. This project is in a bit of flux as I don't work at Squarespace anymore and Squarespace isn't intending to add new features to pgbedrock as it meets their use case, so as long as new functionality doesn't break the existing vanilla postgres functionality, I think incremental additions aren't going to rock any boats.

Edit: also note that the unit tests failed. It looks like some object is currently a str that shouldn't be.

zamai commented 5 years ago

Hey, @zmarine thanks for your reply. Unfortunately, I will not be working on this issue anymore :( Just wanted to give you heads up

zcmarine commented 5 years ago

Hey @zamai, ok, no worries! Thanks for letting me know. I'll close this PR for the moment then.