Skip to content
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

Merged

Conversation

xsellier
Copy link

@xsellier xsellier commented Apr 4, 2016

  • Update postgresql provider's documentation
  • Enforce default value to 'require' for sslmode

@xsellier xsellier force-pushed the feature/postgresql-custom-ssl branch from c82ec04 to 29b6c31 Compare April 4, 2016 15:41
@xsellier xsellier changed the title - Add support for 'sslmode' options present in lib/pq provider/prostgresql: Add support for 'sslmode' options present in lib/pq Apr 4, 2016
@xsellier xsellier changed the title provider/prostgresql: Add support for 'sslmode' options present in lib/pq provider/postgresql: Add support for 'sslmode' options present in lib/pq Apr 4, 2016
@xsellier xsellier force-pushed the feature/postgresql-custom-ssl branch 2 times, most recently from 55af7a3 to 92142d1 Compare April 4, 2016 16:43
@xsellier
Copy link
Author

xsellier commented Apr 4, 2016

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{
Copy link
Contributor

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?

Copy link
Author

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).

Copy link
Author

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

Copy link
Contributor

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.)

Copy link
Author

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 :)

@xsellier xsellier force-pushed the feature/postgresql-custom-ssl branch from 92142d1 to fc9825e Compare April 11, 2016 17:20
- Update psotgresql provider's documentation
- Enforce default value to 'require' for ssl_mode
@apparentlymart
Copy link
Contributor

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.

@apparentlymart apparentlymart self-assigned this Apr 11, 2016
@apparentlymart apparentlymart merged commit fc9825e into hashicorp:master Apr 17, 2016
@apparentlymart
Copy link
Contributor

Thanks again @xsellier, and sorry for the delay on reviewing and merging this.

After familiarizing myself with the available sslmode options in the documentation I pushed a follow-up commit 220d73f to make prefer the default for now, since it seems safest to not unilaterally change the behavior people were seeing in earlier versions, and our usual policy is to keep as close as possible to the behavior of the underlying system to reduce surprises.

@xsellier xsellier deleted the feature/postgresql-custom-ssl branch April 17, 2016 16:00
@ghost
Copy link

ghost commented Apr 26, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants