-
Notifications
You must be signed in to change notification settings - Fork 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
Allow components to be individually activated #10
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.
LGTM 👍. One comment on outputs.
config/eip.tf
Outdated
vpc = true | ||
public_ipv4_pool = "amazon" | ||
tags = merge(tomap({ "Name" : "${var.eks_cluster_name}-loadbalancer-eip" }), var.common_tags) | ||
} | ||
|
||
output "radar_base_eip_allocation_id" { | ||
value = aws_eip.cluster_loadbalancer_eip.allocation_id | ||
value = aws_eip.cluster_loadbalancer_eip != [] ? aws_eip.cluster_loadbalancer_eip[0].allocation_id : "" |
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.
Is value = var.enable_eip? aws_eip.cluster_loadbalancer_eip[0].allocation_id : null
more explicit than checking the empty list? Also, null
drops the output if the resource is disabled so no outputs will be created with empty-string values. Similar comments apply to other places.
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 this error when trying to check the condition:
│ Error: Incorrect condition type
│
│ on eip.tf line 10, in output "radar_base_eip_allocation_id":
│ 10: value = aws_eip.cluster_loadbalancer_eip ? aws_eip.cluster_loadbalancer_eip[0].allocation_id : ""
│ ├────────────────
│ │ aws_eip.cluster_loadbalancer_eip is empty tuple
│
│ The condition expression must be of type bool.
I'll change the values from ""
to null
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 meant checking var.enable_eip
rather than aws_eip.cluster_loadbalancer_eip
?
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 see, I get this error message:
╷
│ Error: Invalid index
│
│ on s3.tf line 142, in output "radar_base_s3_output_bucket_name":
│ 142: value = var.enable_s3 != [] ? aws_s3_bucket.intermediate_output_storage[0].bucket : null
│ ├────────────────
│ │ aws_s3_bucket.intermediate_output_storage is empty tuple
│
│ The given key does not identify an element in this collection value: the collection has no elements.
╵
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.
You have declared var.enable_s3
in variables.tf as a boolean rather than list.
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.
Ah sorry, I was multitasking and didn't pay attention
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 and nice work! I will do some tests on our site once it is merged.
… into optional-components * 'main' of github.com:phidatalab/RADAR-K8s-Infrastructure: update readme and add validation on cluster names differentiate resources by the cluster name
The goal of this PR is to allow components to be individually enabled and customized based on the situation. I'm waiting for #7 to merge to test this but so far running
terraform plan
runs without error and shows expected results. Would be great if you could also test it in your current infrastructure.I feel like the way that I have done it is kinda hacky but I don't know a better way to implement such feature in Terraform, suggestions are welcome!