-
Notifications
You must be signed in to change notification settings - Fork 612
Only require password when used #1356
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about this use case. If you already define the user elsewhere, can't you simply use postgresql::server::database
in the later places? What additional benefit is there in this defined type?
I see a tablespace relation, which is also in postgresql::server::database
.
Then there is a GRANT
. That isn't in postgresql::server::database
, except for an owner. AFAIK that already implies all privileges so the only edge case is if you want that grant.
Am I missing anything?
Well the major use case from my end is "this is how we happen to use it, and it stopped working when d878d13#diff-09acae987abfb6f2de2796ebbbcee8911d8326cf762451972538f4f78984f35c was introduced". |
Oh, and right now, as a workaround, we're passing around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly because it makes the code (even more) compile order dependent. If the inclusion happens in another order, it may fail because no password is provided. Then again, you could already have code where there are 2 different passwords specified and the real password used ends up being determined by the compile order.
Now looking deeper, I see postgresql::server::role
can create a user without a password. So I think this is valid if you drop the fail()
part. That should then also be reflected in the @param
tags.
If the if in line 39 evaluates to false, the user has been created elsewhere, and we don't actually use (and thus need) password in `postgresql::server::db` class.
Done :) |
Did you forget to push? |
90e1522
to
4812333
Compare
Yes, yes I did :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david22swan this would have been nice to get in the release. However, I can't merge it because of a failed test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Will get it merged in now
If the if in line 39 evaluates to false, the user has been created
elsewhere, and we don't actually use (and thus need) password in
postgresql::server::db
class.