-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for environmentd extra args #46
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
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.
Pull Request Overview
This PR adds support for environmentd extra arguments in the Materialize instance configuration.
- Introduces a new optional variable "environmentd_extra_args" in variables.tf.
- Implements logic in main.tf to translate extra args into the required command-line format.
- Updates README.md to document the new configuration option.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
variables.tf | Introduces new optional variable for environmentd extra args. |
main.tf | Formats and passes the extra args through the Kubernetes manifest. |
README.md | Updates documentation for the Materialize instance configuration. |
main.tf
Outdated
format_env_args = { | ||
for instance in var.instances : instance.name => | ||
length(lookup(instance, "environmentd_extra_args", [])) > 0 ? [ | ||
for item in instance.environmentd_extra_args : | ||
"--system-parameter-default=${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.
I'm not sure about this:
not all values in environmentd_extra_args
need to be system parameter defaults, it could be a non system parameter flag as well.
I think we should either make users input a list[str] like
environmentd_extra_args:
- --arg1=x
- --arg2=y
or take in a dict of system_parameter_defaults
more specifically
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.
Oh yes, good point. Let me know what you think of the last change. Essentially it will be up to the user to specify this in the correct format similar to how we have it with the emulator.
environmentd_extra_args = [
"--system-parameter-default=max_clusters=100",
"--system-parameter-default=max_connections=100"
]
@@ -77,6 +77,7 @@ variable "instances" { | |||
name = string | |||
value = string | |||
})), []) | |||
environmentd_extra_args = optional(list(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.
Oh neat! Is this what we'd uses to set, for example, enable_create_table_from_source = true;
or something else?
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.
exactly!
environmentd_extra_args:
- --system-parameter-default=enable_create_table_from_source=true
There's a side effort I've got to let us do this via config files, that would allow us to do this without restarting environmentd, but it's not ready yet.
As a follow-up to #43 which only added support for extra environmentd env vars.