-
Notifications
You must be signed in to change notification settings - Fork 82
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
Updated to support Terraform 1.3.X and latest #431
Conversation
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.
Left some comments. Any particular reason for 1.2.X
instead of 1.3.X
?
@shubydo Think we may be good to merge this now? |
… policies to it if using a custom bucket for nuke config
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.
LGTM. Thanks for the contribution @martwana !
There's a status check that is still pending that appears to not be triggering the current pipeline that runs during PRs. This may be due to the current pipeline not automatically running for forked branches or another reason (given my current understanding). Once we can get that triggered and it passes, let's merge this.
CC: @jrsholly
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Ah, your Azure Pipeline needs updated to use the new terraform 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.
Current pipeline(s) failed because an earlier version of terraform is being used. Can you update the Terraform version in the following files as well?
-
Line 12 in 7a22c7f
TERRAFORM_VERSION: 0.12.31 -
dce/pipelines/destroy-pr-env.yml
Line 47 in 7a22c7f
terraformVersion: "0.12.18"
I'd suggest using the version that matches the new version constraint you defined here
…ng individual values. Co-authored-by: Jack Shubatt <shubydo777@gmail.com>
Can this be merged? |
@shubydo What are the steps left to take to get this into master. I'd really like to ditch my fork and start tracking Optum/dce. |
@martwana sorry for the very delayed response. I am no longer actively on this project, so I'll defer to @dilip0515. @dilip0515 since this is a larger, potentially breaking, change for certain users if they are pointing at the |
As someone "coming from the internet" and a new user of DCE, I would like to nudge this conversation, as my organisation would benefit from this PR being merged as well. FWIW the terraform changes LGTM and the README changes are also complete. |
modules/gateway.tf
Outdated
change_trigger = sha256(data.template_file.api_swagger.rendered) | ||
triggers = { | ||
redeployment = sha1(jsonencode(aws_api_gateway_rest_api.gateway_api.body)) | ||
} |
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.
@martwana it seems like the syntax here isn't working:
❯ terraform plan
╷
│ Error: Incorrect attribute value type
│
│ on .terraform/modules/dce/modules/gateway.tf line 115, in resource "aws_api_gateway_deployment" "gateway_deployment":
│ 115: variables = {
│ 116: // API Changes won't get deployed, without a trigger in TF
│ 117: // See https://medium.com/coryodaniel/til-forcing-terraform-to-deploy-a-aws-api-gateway-deployment-ed36a9f60c1a
│ 118: // and https://github.com/terraform-providers/terraform-provider-aws/issues/162#issuecomment-475323730
│ 119: triggers = {
│ 120: redeployment = sha1(jsonencode(aws_api_gateway_rest_api.gateway_api.body))
│ 121: }
│ 122: }
│
│ Inappropriate value for attribute "variables": element "triggers": string required.
fixed (or removed the error on plan) by removing the "reployment" key:
resource "aws_api_gateway_deployment" "gateway_deployment" {
rest_api_id = aws_api_gateway_rest_api.gateway_api.id
variables = {
triggers = sha1(jsonencode(aws_api_gateway_rest_api.gateway_api.body))
}
lifecycle {
create_before_destroy = true
}
}
This change is sourced from Optum#431
I am also hoping this gets merged soon, 0.13.7 is too old for our use case |
This pr has been accomplished at least in part by the prs:
@martwana If there is something you still want merged be free to request my review. Otherwise I'll look to close this as accomplished in by other prs. |
Closing due to inactivity. LMK if you there is still something you want merged from this pr |
Proposed changes
Updated to support Terraform > v1.3.X. A few AWS resources that generated warnings have also been updated.
Swapped from data.template_file for templatefile() as there is no M1 support for the templates provider.
Added a TF output for the CodeBuild IAM role. If using a custom nuke config in a custom S3 bucket, you need to be able to grant this role access to the bucket. By outputting the name, you can create and IAM policy and attach it to the role.
Types of changes
Checklist
README.md
, inline comments, etc.)CHANGELOG.md
under a## next
release, with a short summary of my changesRelevant Links
N/A
Further comments
N/A