-
Notifications
You must be signed in to change notification settings - Fork 0
Add Helm and CRD override variables for full customization #121
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
base: main
Are you sure you want to change the base?
Conversation
f6c168a to
e83a36e
Compare
|
this looks promising will take a look in detail on monday |
alex-hunt-materialize
left a comment
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.
This is a great idea, but I really don't want us manually parsing and building HCL. Terraform supports json, or we could use a proper HCL parser. I lean toward just converting it all to json, as that opens the door for customers to use their own tools for this too.
I didn't review most of this PR, as it is just a draft. I only did a quick skim of the generator code.
|
|
||
| # Rollout configuration | ||
| force_rollout = var.force_rollout | ||
| request_rollout = var.request_rollout |
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.
We should get consistent about how we handle this stuff. These are removed, while most of the others are marked as deprecated.
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.
yeah, marking this as deprecated is probably for the best.
Great ideas I'll give both a shot. |
6a3990f to
63cb495
Compare
|
@alex-hunt-materialize I went down a rabbit hole with variables in json.tf... the variable type ends up being a string with horrific formatting. We could also use python-hcl2 to output hcl, but it also has shit formatting for complex variables. Hand rolling hcl gen and formatting is potentially the ugliest option to code up, but it generates the cleanest output which I think is worth prioritizing. |
ec06ba6 to
83e2a04
Compare
Introduces generate_terraform_types.py which dynamically generates Terraform variable type definitions from Materialize's upstream schemas: - CRD types from materialize_crd_descriptions.json - Helm parameter types from materialize_operator_chart_parameter.yml The script reads the version from the environmentd_version variable default in the source code and outputs native HCL format (.gen.tf) with proper formatting via terraform fmt. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add check_schema_sync.py to verify generated types match upstream schemas - Add GitHub Actions workflow to run sync check on PRs - Update CONTRIBUTING.md with instructions for regenerating types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Generated type definitions:
- kubernetes/modules/materialize-instance/crd_variables.gen.tf
- {aws,azure,gcp}/modules/operator/helm_variables.gen.tf
- {aws,azure,gcp}/examples/simple/override_variables.gen.tf
Additional changes:
- Add helm_values_override and materialize_spec_override variables to
operator modules and examples for full customization
- Add deprecated force_rollout and request_rollout variables to examples
for backwards compatibility, merged into materialize_spec_override
- Update materialize-instance module to use the new override variable
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
83e2a04 to
a1e37fb
Compare
alex-hunt-materialize
left a comment
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.
Overall this looks pretty good. I do worry a bit about how we will test this, as it is a pretty significant change.
| if crd_type in type_lookup: | ||
| # Detect cycles to prevent infinite recursion | ||
| if crd_type in visited: | ||
| return "any" |
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.
Are we actually hitting cycles? Should we log something here, so we can fix that?
| if len(parts) == 2: | ||
| value_type = crd_type_to_terraform(parts[1], type_lookup, visited) | ||
| if value_type == "string": | ||
| return "map(string)" |
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.
These two lines are useless, as they evaluate to the same as the line below.
| affinity = optional(any) | ||
| defaultResources = optional(any) | ||
| enabled = optional(bool) | ||
| nodeSelector = optional(any) |
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.
Many of these are labeled any. We should at least be able to indicate object(any) for the ones that are objects, even if we can't easily say they are a map of string to string.
I have an update to the auto-generated helm values documentation in MaterializeInc/materialize#34563 which may help with this, if you decide to use the helm-docs generated file.
| environmentdExtraEnv = length(var.environmentd_extra_env) > 0 ? [{ | ||
| name = "MZ_SYSTEM_PARAMETER_DEFAULT" | ||
| value = join(";", [ | ||
| for item in var.environmentd_extra_env : | ||
| "${item.name}=${item.value}" | ||
| ]) | ||
| }] : 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.
Maybe not something to fix in this PR, but this seems like it is wrong and was wrong before this PR. The environmentd_extra_env should not be assumed to be setting a default parameter, but any environment variable.
| "string": "any", # string in helm params often means complex YAML that's stringified | ||
| "bool": "bool", | ||
| "int": "number", | ||
| "object": "any", # object without more info maps to any |
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.
Shouldn't this map to object(any)?
| # Run terraform fmt to ensure proper formatting | ||
| run_terraform_fmt(file_path) | ||
|
|
||
| return True # We always write, so always return True for simplicity |
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.
Why even have these return a bool?
Auto generating terraform variables for our CRD and Helm chart is something I've been thinking about for a while and this is a proof of concept for it.
There are many benefits to having an object or set of vars that work with LSP or AI in order to help write code (as opposed to
object(any)). And it's also extremely beneficial to be able to set any viable attribute for the helm chart or the materialize CR.PR has been updated to be a bit more reviewable.
Commit 1 - adds generation script
Commit 2 - adds GH action and docs
Commit 3 - Adds the auto-generated files and adds deprecations