Refactor resource name definitions and Add Optional Helm Install Support #15
Conversation
bobbyiliev
force-pushed
the
refactor-resource-name-definitions
branch
from
9ffd65f to
d7c67fa
Compare
bobbyiliev
changed the title
| # TODO: Publish the chart to a public repository, currently using a forked version of the chart | ||
| repository = "https://raw.githubusercontent.com/bobbyiliev/materialize/refs/heads/helm-chart-package/misc/helm-charts" |
There was a problem hiding this comment.
The main next step here is to decide on how we would like to publish the Helm Chart.
Currently, the PR uses a temporary solution with a forked version of the Materialize Helm chart.
We need to decide on an appropriate public repository for publishing the Helm chart, this could totally live in the main repository, we would just need an extra flow to package and generate the index.yaml:
helm package operator
helm repo index .
There was a problem hiding this comment.
I vote for main repository because that's easiest for having compatibility and running e2e tests immediately when something in the terraform plugin changes.
There was a problem hiding this comment.
Yes, this should be doable, in this case, we could use GitHub pages as suggested on the Helm website: https://helm.sh/docs/topics/chart_repository/#github-pages-example
modules/operator/main.tf
Outdated
| spec = { | ||
| containers = [{ | ||
| name = "init-db" | ||
| image = "postgres:latest" |
There was a problem hiding this comment.
is there value in pinning this to a version that matches RDS? or not worth the maintenance effort
There was a problem hiding this comment.
This is a good idea! I will update it
| value = format( | ||
| "postgres://%s:%s@%s/%s?sslmode=require", | ||
| each.value.database_username, | ||
| each.value.database_password, |
There was a problem hiding this comment.
this means the pw will appear in plaintext on the Job's definition. you could inject the database username / pw / host as env vars from the Secret (which should be in the same namespace) and construct the URL in the psql statement
| } | ||
| } | ||
|
|
||
| resource "kubernetes_manifest" "db_init_job" { |
There was a problem hiding this comment.
if it works it works, but I'm a bit surprised we need to do this versus using the RDS provider to initialize the databases for us. this is more portable though!
There was a problem hiding this comment.
Yes, it would have been much more convenient, but it is not supported with the RDS terraform module.
You can only initialize a single db:
https://registry.terraform.io/modules/terraform-aws-modules/rds/aws/latest#input_db_name
| materialize_instances = [ | ||
| { | ||
| name = "analytics" | ||
| instance_id = "12345678-1234-1234-1234-123456789012" |
There was a problem hiding this comment.
I was using those in the bucket definitions: https://github.com/MaterializeInc/terraform-aws-materialize/pull/15/files/8edf57c29a39838a5158d788a3dfd04e28e84573#diff-885a9176a02d8454724784f56e5cb4e4c3be25ab53b298a7ff2a8cf4e316d7cbR76
But now thinking about this, it is quite unnecessary, we could just use the name instead.
|
Very cool! This is going to be so nice for getting set up 🌟 |
This PR includes a few major changes:
Global Resource Prefix: this is a breaking change so we will have to adjust the CI and the existing docs:
cluster_name,environment,vpc_name,bucket_name,service_account_name,db_identifier).namespaceandenvironmentonly, which will construct the names of all resources using the format${namespace}-${environment}-${resource_name}.Optional Helm Installation for Materialize Operator:
Fixes #14
Fixes #16