-
Notifications
You must be signed in to change notification settings - Fork 0
First iteration #1
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
Conversation
EvyBongers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps best to also add a README
| required_providers { | ||
| postgresql = { | ||
| source = "cyrilgdn/postgresql" | ||
| version = "1.14.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the requirement for pinning the exact version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primarily because it was copy/pasted from https://registry.terraform.io/providers/cyrilgdn/postgresql/latest/docs 😉
Also, in a next iteration all resource arguments will (most likely) be configured explicitly (no more relying on upstream defaults). This "requires" pinning to a specific version to avoid "drift" (new arguments being introduced for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Development docs state that modules should only restrict minimum version.
Do not use
~>(or other maximum-version constraints) for modules you intend to reuse across many configurations, even if you know the module isn't compatible with certain newer versions. Doing so can sometimes prevent errors, but more often it forces users of the module to update many modules simultaneously when performing routine upgrades.
https://www.terraform.io/language/providers/requirements#version-constraints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get what they're getting at there. But I'm also leaning towards keeping this as-is, as the given recommendation is mostly aimed at large scale deployments. In our case I think having the little bit of additional control over the version actually benefits us. Can always make it more liberal later on when needed. The reverse would likely be way less trivial.
| required_providers { | ||
| postgresql = { | ||
| source = "cyrilgdn/postgresql" | ||
| version = "1.14.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Development docs state that modules should only restrict minimum version.
Do not use
~>(or other maximum-version constraints) for modules you intend to reuse across many configurations, even if you know the module isn't compatible with certain newer versions. Doing so can sometimes prevent errors, but more often it forces users of the module to update many modules simultaneously when performing routine upgrades.
https://www.terraform.io/language/providers/requirements#version-constraints
| @@ -0,0 +1,484 @@ | |||
| Output: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention the caveat that this module and required provider config can only be added once the referred database instance has already been created? Either here or in the README at repo root.
yields: