Refactor to modular structure #69
Conversation
bobbyiliev
changed the title
examples/simple/outputs.tf
Outdated
| dns_name = nlb.nlb_dns_name | ||
| } | ||
| arn = try(module.materialize_instance.nlb_arn, null) | ||
| dns_name = try(module.materialize_instance.nlb_dns_name, null) |
There was a problem hiding this comment.
So ...even though
module.materialize_nlb[0].aws_lb.nlb: Still creating... [2m40s elapsed]
module.materialize_nlb[0].aws_lb.nlb: Creation complete after 2m42s [id=arn:aws:elasticloadbalancing:us-east-1:400121260767:loadbalancer/net/kay-test/1ebf71a043aa1cf0]
the output has:
nlb_details = {
"arn" = null
"dns_name" = null
}
There was a problem hiding this comment.
Also, as an aside ... should we rename cluster_certificate_authority_data to mention public_key part or something? Not sure.
There was a problem hiding this comment.
Ah yes good catch about the nlb details, I pushed a change for this. Will look into the certificate authority data suggestion as well!
| variable "helm_repository" { | ||
| description = "Repository URL for the Materialize operator Helm chart. Leave empty if using local chart." | ||
| type = string | ||
| default = "https://materializeinc.github.io/materialize/" |
There was a problem hiding this comment.
Double checking:
-
With the new change, once this change is in, we'll want to, as part 2, update the self-managed release thing that Dennis showed to bump up this operator_version as well as no longer have the terraform-helm release bump trigger terraform-aws bumps, yes?
-
And, is the plan that whenever the other cloud terraforms are updated, will we, at that time, remove terraform-helm-materialize? (or, we haven't quite decided upon that yet?)
There was a problem hiding this comment.
Yep! This is exactly right!
-
We'll need to update Dennis’s workflow to align with the new restructured repo
-
Also the CI tests should be updated to use the latest operator version
-
Might be good to keep the old version on a separate
ltsbranch in case some users still need it -
We'll most likely remove or deprecate the
terraform-helm-materializemodule, since we want to get rid of the Materialize instances from it, and without those, the module isn't very useful -
We've got this in the design doc here: https://github.com/MaterializeInc/cloud/pull/11040
examples/simple/outputs.tf
Outdated
| dns_name = nlb.nlb_dns_name | ||
| } | ||
| arn = try(module.materialize_nlb.nlb_arn, null) | ||
| dns_name = try(module.materialize_nlb.nlb_dns_name, null) |
There was a problem hiding this comment.
arn = try(module.materialize_nlb[0].nlb_arn, null)
dns_name = try(module.materialize_nlb[0].nlb_dns_name, null)
The output will be a list because we use count variable to create nlb, as of now this will always output null
| cluster_oidc_issuer_url = module.eks.cluster_oidc_issuer_url | ||
| s3_bucket_arn = module.storage.bucket_arn | ||
| use_self_signed_cluster_issuer = true | ||
|
|
There was a problem hiding this comment.
pass enable_disk_support variable from here, if user disables disk_support, that would only be reflected in node-group as per current config. We should reflect the changes in operator as well.
|
|
||
| # Requires OpenEBS to be installed | ||
| disk_config = { | ||
| run_disk_setup_script = var.enable_disk_support ? lookup(var.disk_support_config, "run_disk_setup_script", true) : false |
There was a problem hiding this comment.
Unused configs .
run_disk_setup_script
local_ssd_count
openebs_version
openebs_namespace
| oidc_provider_arn = module.eks.oidc_provider_arn | ||
| cluster_oidc_issuer_url = module.eks.cluster_oidc_issuer_url | ||
| s3_bucket_arn = module.storage.bucket_arn | ||
| use_self_signed_cluster_issuer = true |
There was a problem hiding this comment.
why not use_self_signed_cluster_issuer = var.install_materialize_instance? Just like we do it in certificates module.
| }, | ||
| cloudProvider = { | ||
| type = "aws" | ||
| region = var.aws_region |
There was a problem hiding this comment.
we can get this from https://registry.terraform.io/providers/-/aws/latest/docs/data-sources/region, just a nit, we will have the region var in the main module always, so eventually doesn't matter.
| instances = module.operator[0].materialize_instances | ||
| instance_resource_ids = module.operator[0].materialize_instance_resource_ids | ||
| } : null | ||
| value = module.operator.materialize_s3_role_arn | ||
| } | ||
|
|
There was a problem hiding this comment.
missing outputs of operator and materialize instance
This PR primarily restructures the codebase by splitting it into standalone, reusable submodules. No core logic has been changed, the changes are mostly related to moving and organizing code.
Materialize instances are now modeled as 1:1 with associated resources, instead of 1:N. This gives users more flexibility to define and manage instances independently.
Also removed usage of the external Materialize Helm module.
The root module is now just an example, and end users are expected to compose the submodules as needed.