cyrilgdn / terraform-provider-postgresql

Terraform PostgreSQL provider
https://www.terraform.io/docs/providers/postgresql/
Mozilla Public License 2.0
355 stars 180 forks source link

Add support for view resource #442

Open nguyen1tech opened 1 month ago

nguyen1tech commented 1 month ago

This PR adds support for views in the provider.

Features:

Notes: This PR is only for regular views, materialized views deserve a separate resource type. eg: _postgresql_materializedview

Please let me know if you see any issues!

AndrewJackson2020 commented 1 month ago

Thanks for putting this together, it looks great and I hope this gets merged.

One potential issue that I see is that sometimes views cannot be replaced with a mere create or replace statement. See below for example. Is this resource resilient to this issue? The quick and dirty way to make it resilient to it is to issue a DROP VIEW IF EXISTS command before the CREATE VIEW command. This should have very little downtime as the query can be dropped and created very quickly. The more complete method would be to parse the change and issue ALTER VIEW commands, granted this would take a lot more work.

127.0.0.1:25432 postgres@postgres=#
create or replace view test_view as SELECT 1 as one;
CREATE VIEW
127.0.0.1:25432 postgres@postgres=#
create or replace view test_view as SELECT 2 as two;
ERROR:  cannot change name of view column "one" to "two"
HINT:  Use ALTER VIEW ... RENAME COLUMN ... to change name of view column instead.
nguyen1tech commented 1 month ago

Thank you @AndrewJackson2020 for your review. The issue is fixed. I'll be great if you can take a look again!

AndrewJackson2020 commented 1 month ago

Thank you @AndrewJackson2020 for your review. The issue is fixed. I'll be great if you can take a look again!

Looks good, I would just throw in a unit test that covers the case I outlined above. Also I would get rid of the "create or replace" since ur function is already deleting it so the "or replace" part is not necessary anymore.

Was also looking at your implementation of read. The comments make it sound like if there is any drift that was not caused by terraform then manual intervention is required. Is this the case? If so why?

nguyen1tech commented 1 month ago

Thank you @AndrewJackson2020 for having a look. For the drift, it might happen when multiple people have modified access to the view and they just directly modify the view in Postgres without Terraform awareness. If it happens I think we should let them(the people who make the change and the people who manage Postgres via Terraform) decide whether to keep the change in Postgres or replace it with what in Terraform.

AndrewJackson2020 commented 1 month ago

Thank you @AndrewJackson2020 for having a look. For the drift, it might happen when multiple people have modified access to the view and they just directly modify the view in Postgres without Terraform awareness. If it happens I think we should let them(the people who make the change and the people who manage Postgres via Terraform) decide whether to keep the change in Postgres or replace it with what in Terraform.

So its just a warning? Nothing prevents the user from overriding the changes with terraform. I think that makes sense. I think that is all I have for your PR hopefully the maintainer will be able to review and merge.

nguyen1tech commented 1 month ago

Really appreciate your review! Thank you @AndrewJackson2020

aminehmida commented 2 weeks ago

@cyrilgdn & @nguyen1tech thanks both for working on this module. It's super useful and I am really loving it so far. @cyrilgdn Any chance this can be merged if there are no other issues please?

Thanks all for your time spent on this ❤️ Happy to help if there is anything I can do to expedite the review for this PR.