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

feat: Add support for materialized_view #139

Merged
merged 22 commits into from
Mar 18, 2022

Conversation

mwallace582
Copy link
Contributor

Hi Everyone,

I've been really enjoying these opinionated terraform modules from Google, thank you for maintaining them!

I just discovered that materialized views were not supported by this module while refactoring some code, and saw an open pull request. I figured I could help by performing the requested rebase.

Please let me know if this separate pull request is not an appropriate way to get this work merged. I definitely don't want to take credit for the work that @jogoldberg has done here.

Thanks!
Matthew

@comment-bot-dev
Copy link

comment-bot-dev commented Feb 8, 2022

Thanks for the PR! 🚀
✅ Lint checks have passed.

@mwallace582
Copy link
Contributor Author

Hm, I'm not quite sure what to do about the CLA checks for the ubuntu​@ip-172-31-36-97.eu-central-1.compute.internal user. The commit authored by this user is simply generating docs.

Some help would be appreciated here. I'm happy to make any necessary changes.

@morgante
Copy link
Contributor

@mwallace582 You need to rebase or somehow fix the email. http://treeindev.net/article/git-change-commit-name

mwallace582 and others added 7 commits March 17, 2022 14:58
Add trailing whitespace

Fix location of materialized_view

Switch to lookup to prevent materialized_view from being requiired

Fix missing comma

tf_fmt

Fix default value

Generate Docs

Update tests

tf_fmt
@mwallace582
Copy link
Contributor Author

Thank you for the help @morgante! I believe that this is ready to go now. I'd be happy to make any changes necessary if that's not the case.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I think we could actually improve clarity here by separating out from tables. Even though the underlying implementation uses tables, they're conceptually different.

I suggest we add a new materialized_views variable (at the top level) and create the resources in a similar way to how we do the views variable today.

* Move materialized_views into a variable
* Clean up file with bad line-endings
@mwallace582 mwallace582 requested a review from morgante March 18, 2022 20:00
variables.tf Show resolved Hide resolved
@mwallace582 mwallace582 requested a review from morgante March 18, 2022 22:12
README.md Outdated Show resolved Hide resolved
@morgante
Copy link
Contributor

Also need to regenerate docs. This comment keeps up to date: #139 (comment)

@morgante morgante merged commit e663c25 into terraform-google-modules:master Mar 18, 2022
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