Closed gmr closed 6 years ago
Hey Gavin, thanks for putting in a PR!!
Ignoring fields
I think it would be cleaner if we just explicitly specify the fields we want here. That is the query used in both pgbedrock generate
and pgbedrock configure
to get the current state of roles, so if we hard-coded the fields we want from there we would just never select that rolcatupdate
field (or any other differing fields from historical versions). This also requires no changes to how pgbedrock works or additional testing, which is nifty.
Adding a --role
Parameter
I'll admit that the use case for this didn't make sense to me until I read the description in pg_dump's reference page, but now I agree that that would be useful.
My one concern is that we use SET ROLE
and RESET ROLE
within the privileges.py
section since default privileges are based on which role granted the privilege. I've confirmed that if you start as role A, set role to B, then set role to C, then reset role you will end up back as A, which is not what we'd want here, so we'd need to modify these statements to use the role that was passed in.
In addition, even though I generally like to emulate the APIs that people are already familiar with (e.g. pg_dump, in this case), I think the flag --role
is a bit confusing given that this tool is about managing roles; at first blush I thought this was a flag for a new submodule, and given that there are flags like --attributes
and --memberships
, I could see other people getting confused as well. I like your point about how this is basically like sudo
ing, so maybe something like --sudo-role
plus copying over the explanation from the pg_dump reference page into the docstring (both function + click-level) would make it more intuitive.
And again, thanks for contributing new ideas and your time! I'm excited to get these changes added in. As a side-note, I'll be on vacation next week so I can't guarantee I'll be able to respond during that time.
Hey Gavin,
I'm closing this PR, but if you feel strongly about having either of these features added then please add an issue!
In the couple months since this PR was put in we've added a mechanism to formally support different Postgres version and have added formal support for Postgres 9.5, 9.6, and 10. If getting Postgres 9.4 support online is necessary for your use case that should be doable, especially as we start generalizing the tool to facilitate Redshift, which will similarly require differentiating the queries we run based on the implementation we're running against.
Similarly, if having a --set
option is critical, put in an issue. I'm still less clear on the implications of that, but we could talk about how we could build and test that if you feel strongly on it.
Sorry it took a while to circle back on this. I really appreciate your ideas and would love to have you involved in the project!
This PR adds two things:
Adds a --role CLI argument that will triggers pgbedrock to
SET ROLE
prior to performing any database operations. This mirrors the argument inpg_dump
and is used similar to asu
orsudo
type functionality. See SET ROLEAdds a new list called IGNORE in attributes that lists columns to ignore. Prior to 9.5 there was an attribute in
pg_authid
calledrolcatupdate
. While I only needed to ignore that one, I figured having a list of things to ignore would be a solid thing to do for future issues of a similar nature. In addition, I changed the dict references that could cause aKeyError
if an unknown column was encountered to just return the column name.I didn't see much opportunity to cleanly add test coverage for these options, but if coverage would help get the PR merged and a new version released with these additions, I'd be happy to provide it.
Thanks for an interesting tool!