Conversation
| # PostgreSQL Settings | ||
| String $superuser_username = 'postgres', | ||
| String $superuser_password = 'changeme', | ||
| Optional[String] $superuser_password = 'changeme', |
There was a problem hiding this comment.
If the default value isn't undef, the Optional type doesn't make sense.
There was a problem hiding this comment.
Modified to set the default to undef then, because when I configure cert auth, I don't want to also accidentally enable password auth with a default dummy password.
Alternatively, I could add some if/else logic that checks if any of the cert auth parameters are set and if so, never set the superuser_password in the conf file?
| Optional[String] $superuser_sslmode = undef, | ||
| Optional[String] $superuser_sslkey = undef, | ||
| Optional[String] $superuser_sslpassword = undef, | ||
| Optional[String] $superuser_sslcert = undef, | ||
| Optional[String] $superuser_sslrootcert = undef, |
There was a problem hiding this comment.
Do we want to allow empty strings as valid values? Otherwise you can enforce the string length by using Optional[String[1]]. For the paths, you can use the Stdlib::Absolutepath type, and for sslmode probably an enum?
| superuser: | ||
| username: <%= @superuser_username %> | ||
| password: '<%= @superuser_password %>' | ||
| <% if @superuser_password != nil -%> |
There was a problem hiding this comment.
you can simplify the check with
| <% if @superuser_password != nil -%> | |
| <% if @superuser_password -%> |
|
thanks for the PR. please also run |
|
Trying to set up some local testing to see why the config isn't validating, but I'm digging myself another rabbit hole. And missing half of CfgMgmtCamp ;-) Will look into it later. |
|
Don't have a test for the tags yet, should I also include that in this PR? EDIT: Nvm, I did, since I needed to fix the quoted password regression anyway. |
|
Is this acceptable? |
2 participants
Pull Request (PR) description
This is basically a rebase of PR #33 with Maarten's IRL approval. I have had this running in production for 3 years now on top of our local mirror. The rebase should make merging easier.
I also see there are more recent PR's for adding tags support. Not sure if mine has preference or not. Or I can split up the commits into two PR's. Just let me know.
This Pull Request (PR) fixes the following issues