postgresql_database: Reassign objects owners if database owner changes #458
Conversation
There was a problem hiding this comment.
Hi @lukaalba,
Thanks for your work on that 💪 (and to add the acceptance tests 🙏 )
I've just allowed myself to push a small modification in the tests to drop the role you created after each test so we can execute the same tests multiple times (without recreating the whole database server)
I have also one question/comment on the main function.
But otherwise it seems good to me 👍
| @@ -476,6 +488,54 @@ func setDBOwner(db *DBConnection, d *schema.ResourceData) error { | |||
| return err | |||
| } | |||
|
|
|||
| func setAlterOwnership(db *DBConnection, d *schema.ResourceData) error { | |||
| if d.HasChange(dbOwnerAttr) || d.HasChange(dbAlterObjectOwnership) { | |||
There was a problem hiding this comment.
if only dbAlterObjectOwnership changes, it seems you will execute the REASSIGN with the same previous and new owner so I'm not sure to see the point on this case 🤔
At least you should check if currentOwner and newOwner are different below.
There was a problem hiding this comment.
Thanks for the review. If updated the code so it checks if currentOwner and newOwner are equal before executing the sql statements
| @@ -393,6 +400,10 @@ func resourcePostgreSQLDatabaseUpdate(db *DBConnection, d *schema.ResourceData) | |||
| return err | |||
| } | |||
|
|
|||
| if err := setAlterOwnership(db, d); err != nil { | |||
There was a problem hiding this comment.
This does more than setting a database "parameter", probably this function could have better name
(something like reassignOwnership or similar)
There was a problem hiding this comment.
Should I update the whole name for the flag then? I don't want to break the naming conventions of the functions called in the resourcePostgreSQLDatabaseUpdate, so I'm not sure if it might be misleading if the function name changes, but the flag itself not
There was a problem hiding this comment.
Hmm I get the point, we can keep this function name to be consistent with the rest of the functions then 👍
| @@ -393,6 +400,10 @@ func resourcePostgreSQLDatabaseUpdate(db *DBConnection, d *schema.ResourceData) | |||
| return err | |||
| } | |||
|
|
|||
| if err := setAlterOwnership(db, d); err != nil { | |||
There was a problem hiding this comment.
Hmm I get the point, we can keep this function name to be consistent with the rest of the functions then 👍
|
I've just fix the used of the transaction (which fixes at the same time the |
cyrilgdn
changed the title
Hi,
this is my first Pull Request, so if something is missing or wrong, let me know.
To resolve 439 I suggest to add an additional parameter to the database resource called
alter_object_ownership. If set to true, it will issue a REASSIGN OWNED BY query on the database when the owner changes, transfer the whole ownership in the database from the previous owner to the new one.As far as I can tell this is only possible if the user in the provider configuration is a superuser or a user which has grants for both the previous owner role and the new one. So I skipped the test in the rds-like test suite.