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

Add support for view resource #442

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nguyen1tech
Copy link

This PR adds support for views in the provider.

Features:

  • Manages Postgres views: create/update/delete.
  • Supports all view's options: check_option, security_barrier, security_invoker.

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

Please let me know if you see any issues!

@AndrewJackson2020
Copy link

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
Copy link
Author

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

@AndrewJackson2020
Copy link

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
Copy link
Author

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

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
Copy link
Author

Really appreciate your review! Thank you @AndrewJackson2020

@aminehmida
Copy link

aminehmida commented Jun 12, 2024

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

@cyrilgdn cyrilgdn force-pushed the main branch 3 times, most recently from f2c2e47 to dea1401 Compare September 8, 2024 17:06
@jvyoralek
Copy link

@cyrilgdn & @nguyen1tech I would like to express my sincere appreciation for the work done on this PR. The functionality it introduces will be incredibly beneficial to my work, and it appears that the implementation is nearly complete. I'm eagerly looking forward to seeing this feature finalized and merged. Thank you for your efforts on this valuable addition to the project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants