-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
update to tf 0.13 #22
Conversation
versions.tf
Outdated
@@ -1,5 +1,5 @@ | |||
terraform { | |||
required_version = "~> 0.12.0" | |||
required_version = ">= 0.12.0, < 0.14" | |||
|
|||
required_providers { | |||
aws = "~> 2.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.
aws = "~> 2.0" | |
aws = ">= 2.0" |
use >=
for all other providers as well
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.
Thanks for the suggestion. Wouldn't this be risky if a breaking change comes along in provider v3 or v4 that makes our module stop working?
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.
there is a risk, but the modules usually work ok with v3.
Otherwise, we'd have to modify all our hundreds of modules again and again for the next versions, which is way more work than to fix a module or a few if they are broken by the new provider 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.
people should pin to a release, so even if the new release breaks something, the previous releases should work
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.
Not necessarily, I actually think that's the danger.
If the consumer is pinning to v1
of the module and the module allows a provider >= 2.0
and a new provider (say v3) is released, the consumer could get v3
of the provider, which could have backward-incompatible changes. The AWS v2
to AWS v3
is a perfect example as there are a bunch of breaking changes.
I'm fine following the convention here but just wanted to make sure I pointed out the risk and discussed the thought process.
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.
yes, we allow all versions, and we know about the risks.
It's still better than to update and test hundreds of modules for each new version of the providers
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.
@mcalhoun LGTM, a few comments:
-
Update
examples/complete
tocontext.tf
similar to https://github.com/cloudposse/terraform-aws-datadog-integration/tree/master/examples/complete -
run these commands from the root of the repo
make init
make github/init
make readme
- Use the new provider syntax (the old one is deprecated and will throw errors in the next TF release
https://github.com/cloudposse/terraform-aws-datadog-integration/blob/master/versions.tf#L4
Thanks for the review @aknysh I'll make those changes now! |
/test all |
/test all |
/test all |
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.
in examples/complete/main.tf
, this
module "sns" {
source = "../../"
namespace = var.namespace
name = var.name
stage = var.stage
}
should be updated to this
module "sns" {
source = "../../"
context = module.this.context
}
see https://github.com/cloudposse/terraform-aws-datadog-integration/blob/master/examples/complete/main.tf
/test all |
|
context.tf
Outdated
# | ||
|
||
module "this" { | ||
source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.19.2" |
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.
source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.19.2" | |
source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.20.0" |
/test all |
what
why