Various changes to the PostgreSQL provider #10682

sean- · 2016-12-12T21:22:42Z

This is basically ready to be reviewed. Before it can be merged one test needs to be fixed (hence the checkpoint, want a pair of eyes) and the version attribute should be added to postgresql_extension (almost done).

CC @kyxap1 and @samdunne who have been doing some PostgreSQL provider work recently.

… idioms. Also don't specify the default and rely on github.com/lib/pq (which uses "required" and is different than what libpq(3) uses, which is "preferred" and unsupported by github.com/lib/pq).

can be removed in Terraform 0.8.0

If they're required and the value is missing, the test will fail. There's no need to enforce that in the test itself.

This will cause someone some grief. TODO: Figure out how to prevent someone from blowing off their foot if they twiddle this after the fact.

…t PGHOSTADDR. https://github.com/lib/pq/blob/d8eeeb8bae8896dd8e1b7e514ab0d396c4f12a1b/conn.go#L1851 This reverts commit 26a2a974f2b2f2cefd41b426b8469fa3ea72b33e.

…slmode`. Both libpq(3) and github.com/lib/pq both use `sslmode`. Prefer this vs the non-standard `ssl_mode`. `ssl_mode` is supported for compatibility but should be removed in the future. Changelog: yes

* Add support to import databases. See docs. * Add support for renaming databases * Add support for all known PostgreSQL database attributes, including: * "allow_connections" * "lc_ctype" * "lc_collate" * "connection_limit" * "encoding" * "is_template" * "owner" * "tablespace_name" * "template"

…ork.

Add "pathological" test.

*Read() and *Update() still need to be updated.

…greSQL.

@stack72

Pointed out by: @stack72

…lly.

stack72 · 2016-12-12T22:48:12Z

@jen20 is going to give this another pair of 👀

it LGTM! I love the addition of tfAppName for tracking. The other change of node is the ssl_mode is now deprecated in-favour of sslmode

% make testacc TEST=./builtin/providers/postgresql                                                                          2 ↵ ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/12 22:41:44 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/postgresql -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccPostgresqlDatabase_Basic
--- PASS: TestAccPostgresqlDatabase_Basic (2.07s)
=== RUN   TestAccPostgresqlDatabase_DefaultOwner
--- PASS: TestAccPostgresqlDatabase_DefaultOwner (1.03s)
=== RUN   TestAccPostgresqlExtension_Basic
--- PASS: TestAccPostgresqlExtension_Basic (0.09s)
=== RUN   TestAccPostgresqlExtension_SchemaRename
--- PASS: TestAccPostgresqlExtension_SchemaRename (0.14s)
=== RUN   TestAccPostgresqlRole_Basic
--- PASS: TestAccPostgresqlRole_Basic (0.29s)
=== RUN   TestAccPostgresqlSchema_Basic
--- PASS: TestAccPostgresqlSchema_Basic (0.24s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/postgresql    3.880s

jen20

A few very minor comments here, but 👍 from me. If the first version is 0.8, now is the time to address removing the environment variables mentioned in three of the comments.

jen20 · 2016-12-12T23:06:35Z

builtin/providers/postgresql/GNUmakefile

@@ -0,0 +1,28 @@
+POSTGRES?=/opt/local/lib/postgresql96/bin/postgres


I think it's worth adding a comment to the top of this file stating the intended use, or use the "self-documenting" structure and make the help target be the default.

jen20 · 2016-12-12T23:08:25Z

builtin/providers/postgresql/provider.go

-				Required:    true,
+				Type:     schema.TypeString,
+				Optional: true,
+				// TODO(sean@): Remove POSTGRESQL_HOST in 0.8


The first release this will see is 0.8 - do we want to remove this now?

I'd be okay with that. I've had this kicking around in some form since the mid-0.7 days. Removing now.

jen20 · 2016-12-12T23:08:45Z

builtin/providers/postgresql/provider.go

+			"username": {
+				Type:     schema.TypeString,
+				Optional: true,
+				// TODO(sean@): Remove POSTGRESQL_USER in 0.8


Same re 0.8 removal

jen20 · 2016-12-12T23:09:08Z

builtin/providers/postgresql/provider.go

-				Optional:    true,
+				Type:     schema.TypeString,
+				Optional: true,
+				// TODO(sean@): Remove POSTGRESQL_PASSWORD in 0.8


Same re 0.8 removal

jen20 · 2016-12-12T23:09:54Z

builtin/providers/postgresql/resource_postgresql_database_test.go

@@ -6,40 +6,101 @@ import (
 	"testing"



Formatting error on imports here

ghost · 2020-04-18T02:31:30Z

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.

sean- added 30 commits December 10, 2016 12:32

Change the PostgreSQL PGSSLMODE option to sslmode to match PostgreSQL…

f31ebff

… idioms. Also don't specify the default and rely on github.com/lib/pq (which uses "required" and is different than what libpq(3) uses, which is "preferred" and unsupported by github.com/lib/pq).

Fall through to using the defaults from github.com/lib/pq

b68ef2c

Add comments requesting that the testing-specific environment variables

ec130d5

can be removed in Terraform 0.8.0

Remove the PGPASSWORD requirement for tests.

655617e

If they're required and the value is missing, the test will fail. There's no need to enforce that in the test itself.

Spell ssl_mode like sslmode

7e5ccc0

Eh, specify default values lib/pq zero initializes itself

2aee081

Flesh out the CREATE DATABASE for PostgreSQL.

f3add9e

Update the FIXME/TODO style to match Google style guides

68cadd3

Update the descriptions to mostly match the official PostgreSQL docs.

deb91f6

Add missing descriptions to owner and name

02dea2e

Factor out the validate function for connection limits

242405b

Decorate the computed attribute where appropriate

547dcf2

ForceNew is required when changing the locale, ctype, and encoding.

5b66bf0

This will cause someone some grief. TODO: Figure out how to prevent someone from blowing off their foot if they twiddle this after the fact.

Use a string instead of the %t modifier for printing a bool in SQL

3779dff

Prefer PGHOSTADDR over PGHOST, if set.

55061d1

Revert "Prefer PGHOSTADDR over PGHOST, if set." lib/pq doesn't suppor…

44d907a

…t PGHOSTADDR. https://github.com/lib/pq/blob/d8eeeb8bae8896dd8e1b7e514ab0d396c4f12a1b/conn.go#L1851 This reverts commit 26a2a974f2b2f2cefd41b426b8469fa3ea72b33e.

Add a fallback_application_name to the PostgreSQL DSN.

602a908

Allow the PostgreSQL provider to talk to a non-default database.

a200899

Depreciate the PostgreSQL provider's ssl_mode option in favor of `s…

3750bf7

…slmode`. Both libpq(3) and github.com/lib/pq both use `sslmode`. Prefer this vs the non-standard `ssl_mode`. `ssl_mode` is supported for compatibility but should be removed in the future. Changelog: yes

Add connect_timeout support to the PostgreSQL provider.

300ef2b

Alpha sort the resources in the PostgreSQL map.

59f4ad6

Commit miss earlier, fix syntax from unstaged commit.

f253fc9

Remove SetId() call from *Read(), this isn't required for import to w…

e9b2b38

…ork.

Always remove the IS_TEMPLATE attribute before dropping a database.

37fdc95

Don't use d.GetOk() when the zero value is a required attribute.

6b540ec

Add "pathological" test.

Remove unused code.

2e52914

Commit WIP re: updated postgresql_role provider.

bfc2a2d

*Read() and *Update() still need to be updated.

Style nit

db5d7b0

Clean up the DatabaseCreate call.

e244847

sean- added 10 commits December 12, 2016 00:53

Remove a duplicate connect_timeout from a rebase+stash.

15cd542

Rename variable from roleSuperUserAttr to roleSuperuserAttr.

b576a3e

Change the default for inherit from false to true to match Post…

e9dc92c

…greSQL.

Change the default for valid_until to infinity to match the default.

e36827c

Enable import support for PostgreSQL's extensions.

6deb61b

Nuke some whitespace.

4c6c52e

Expand postgresql_role support to include all known options.

1a93309

Add PostgreSQL schema support

d1c9ebb

Fix the description for the postgresql_role.

201d9b9

Teach postgresql_extension about schemas.

daa9514

sean- requested a review from stack72 December 12, 2016 21:22

Fix the remaining postgresql_extension unit test.

c602f02

apparentlymart changed the title ~~F fixup postgresql~~ Various changes to the PostgreSQL provider Dec 12, 2016

apparentlymart added enhancement provider/postgresql labels Dec 12, 2016

sean- added 3 commits December 12, 2016 14:30

Fix up helpers.

a4965c0

Pointed out by: @stack72

Add the version attribute to postgresql_extension.

8c41f08

Add the testing Makefile that I'm using for testing the provider loca…

6ed3777

…lly.

stack72 approved these changes Dec 12, 2016

View reviewed changes

jen20 approved these changes Dec 12, 2016

View reviewed changes

sean- added 2 commits December 12, 2016 15:21

Remove non-standard environment variables in prep for 0.8.

2ecd42c

gofmt cleanup on imports.

ce60c4f

jen20 approved these changes Dec 12, 2016

View reviewed changes

jen20 merged commit 7cda9e8 into master Dec 12, 2016

jen20 deleted the f-fixup-postgresql branch December 12, 2016 23:23

ghost locked and limited conversation to collaborators Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various changes to the PostgreSQL provider #10682

Various changes to the PostgreSQL provider #10682

sean- commented Dec 12, 2016

stack72 commented Dec 12, 2016

jen20 left a comment

jen20 Dec 12, 2016

jen20 Dec 12, 2016

sean- Dec 12, 2016

jen20 Dec 12, 2016

jen20 Dec 12, 2016

jen20 Dec 12, 2016

ghost commented Apr 18, 2020

		@@ -0,0 +1,28 @@
		POSTGRES?=/opt/local/lib/postgresql96/bin/postgres

Various changes to the PostgreSQL provider #10682

Various changes to the PostgreSQL provider #10682

Conversation

sean- commented Dec 12, 2016

stack72 commented Dec 12, 2016

jen20 left a comment

Choose a reason for hiding this comment

jen20 Dec 12, 2016

Choose a reason for hiding this comment

jen20 Dec 12, 2016

Choose a reason for hiding this comment

sean- Dec 12, 2016

Choose a reason for hiding this comment

jen20 Dec 12, 2016

Choose a reason for hiding this comment

jen20 Dec 12, 2016

Choose a reason for hiding this comment

jen20 Dec 12, 2016

Choose a reason for hiding this comment

ghost commented Apr 18, 2020