Conversation
Fixes: #5799 Follows the same scheme as the kubernetes secret resolver. To resolve an env var successfully, the string must start with: `${env:` and end with `}`. The env var name then has to start from a valid alphabet, or underscore. The name then may contain numbers. Example: `"table": "${env:TABLE_NAME}"` Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
| Ok(value) => Ok(value), | ||
| Err(env::VarError::NotPresent) | Err(env::VarError::NotUnicode(_)) => { | ||
| Err(SecretRefResolutionError::EnvVarNotSet { | ||
| env_ref: secret_ref, |
There was a problem hiding this comment.
When env::var returns VarError::NotUnicode, the variable is set — it just contains non-UTF-8 bytes. Mapping it to EnvVarNotSet produces a misleading error: the user will check env | grep VAR, see it set, and wonder why Feldera says otherwise.
Consider a separate error variant, or at minimum tweak the message to "is not set or contains non-UTF-8 bytes".
There was a problem hiding this comment.
The separate error variant is preferred.
gz
left a comment
There was a problem hiding this comment.
what happens to checkpointed state with env variables
| - It is not possible to specify a secret value type other than string | ||
| - It is not possible to specify a secret as a substring, for example | ||
| `abc${secret:kubernetes:a/b}def` does not work | ||
| - It is not possible to specify a reference value type other than string |
There was a problem hiding this comment.
create follow up issue for these?
There was a problem hiding this comment.
The implementation itself works, though I'm still in-between whether this adds functionality to Feldera that is out of scope.
The connector configuration was made part of the SQL to have it be self-contained (rather than the very prior version, where the connector was a separate API entity). Adding extra ways to parameterize it, either out-of-band or via the runtime configuration, is effectively going against that design decision.
The user controls the pipeline, and is able to create them in whatever "templating" framework that is useful to them. So if a value should be changed in the connector configuration, then that change can be applied.
Example analogies to this design are Kubernetes vs. Helm, or CSS vs. SCSS.
I think the actual issue this is trying to fix is to avoid recompilation when the connector changes. We could solve that differently, for example changing the way connectors are provided by having them be a separate field and removing it from SQL (or supplementary to it).
| ``` | ||
| can be identified to be secret or environment variable references (this excludes keys), for example | ||
| (secret named `a` at data key `b` has value `value`): | ||
| ``` |
There was a problem hiding this comment.
Yeah, this was just autoformatting by the IDE.
|
The mechanism that this is being added to is called secret references, and has claimed string that start with |
Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com> [ci] apply automatic fixes Signed-off-by: feldera-bot <feldera-bot@feldera.com> apply suggestions from code review Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
|
Also note that |
Signed-off-by: feldera-bot <feldera-bot@feldera.com>
Describe Manual Test Plan
Manually tested by creating a pipeline with env vars in connector config and setting env vars via the pipeline config.
Checklist