-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
provider/postgresql: Add support for 'sslmode' options present in lib/pq #6008
provider/postgresql: Add support for 'sslmode' options present in lib/pq #6008
Conversation
xsellier
commented
Apr 4, 2016
- Update postgresql provider's documentation
- Enforce default value to 'require' for sslmode
c82ec04
to
29b6c31
Compare
55af7a3
to
92142d1
Compare
Compiled and tested locally, it works like a charm 🎱 |
@@ -35,6 +35,12 @@ func Provider() terraform.ResourceProvider { | |||
DefaultFunc: schema.EnvDefaultFunc("POSTGRESQL_PASSWORD", nil), | |||
Description: "Password for postgresql server connection", | |||
}, | |||
"sslmode": &schema.Schema{ |
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.
Normally we separate words with underscores, so I'd expect this to be ssl_mode
.
However, we do make exceptions for things that are expected to be familiar to users of the service a provider is interacting with. So we could leave it as sslmode
if that's how it's named in some visible part of the Postgres "UI", which I think it might be here because it seems like connStr
is something like a DSN that you'd specify when configuring a client. Is that right?
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.
You are right 👍
According to this documentation, it is defined as sslmode
.
By using connStr
, it looks like postgresql://localhost:2345/postgres?sslmode=require
By using lib pq library, it looks like db, err := sql.Open("postgres", "user=postgres dbname=postgres sslmode=require")
But I'll rename it for ssl_mode
if you prefer (I have no preference).
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.
So, should line rename it for ssl_mode
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 think my gut feeling is that it's better for it to be ssl_mode
to match with the style used elsewhere in Terraform. That way users won't need to refer to the docs to remember what style is used for this attribute.
(Sorry for the delay in answering.)
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.
Thank you for your answer !
I've just amended/pushed the modification.
P.S:
Franckly, this delay is acceptable :)
92142d1
to
fc9825e
Compare
- Update psotgresql provider's documentation - Enforce default value to 'require' for ssl_mode
Thanks for this, @xsellier... it looks after a quick pass! I don't have spare cycles to test and merge this right now, but I'm going to assign it to myself to remind me to take a look at it soon. |
Thanks again @xsellier, and sorry for the delay on reviewing and merging this. After familiarizing myself with the available |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |