/{go,integration-tests}: support environment variable interpolation#10267
/{go,integration-tests}: support environment variable interpolation#10267coffeegoddd merged 5 commits intomainfrom
Conversation
|
@coffeegoddd DOLT
|
|
Fixes: #10239 |
|
@coffeegoddd DOLT
|
|
|
||
| func TestYamlConfigFromFileEnvInterpolation_Default(t *testing.T) { | ||
| // Empty env vars are treated as missing for interpolation purposes. | ||
| t.Setenv("DOLT_TEST_SQLSERVER_PORT", "") |
There was a problem hiding this comment.
Hmm. As a user I'd want this to barf. They set a variable that is garbage. I would think the default value (15200) would only be used when the env var wasn't set at all. Is there a common pattern for this behavior you're following? I asked chatgpt and it seems like every tool does this a little differently. I could see the argument either way, just saying what I'd want it to do.
|
|
||
| // envLookupFunc returns (value, true) if the env var exists. | ||
| // Note that an env var may exist but be empty. | ||
| type envLookupFunc func(string) (string, bool) |
There was a problem hiding this comment.
Why this? I don't see anywhere that we create a custom lookup method other than os.LookupEnv.
Seems like we could simplify and drop it.
| // | ||
| // Notes: | ||
| // - Expansion is applied to the input text only (env var values are inserted literally). | ||
| // - Default expressions are expanded (i.e. nested placeholders inside defaults are supported). |
There was a problem hiding this comment.
You should add a restriction (and validity check) that the env var is on one line. You get a pretty gross error when you have a missing bracket, like:
port: ${DOLT_PORT
It basically prints the entire config file. The parser should stop at the newline.
There was a problem hiding this comment.
Also, I tested having no characters after the example above, and I got an error telling me what byte offset the problem was at:
lcl:~/Documents/data_dir_1/db6$ cat config.yaml
listener:
port: ${DOLT_PORT
lcl:~/Documents/data_dir_1/db6$ dolt sql-server --config config.yaml
Failed to resolve the data directory: Failed to interpolate environment variables in yaml file 'config.yaml'. Error: unterminated environment placeholder starting at byte 18
It would be much nicer if it told me what line the error was on. (a real config file has way more than 3 lines, obviously).
| // data[dollarIdx] == '$' and data[dollarIdx+1] == '{' expected. | ||
| start := dollarIdx + 2 // after ${ | ||
|
|
||
| closingBrace := start |
There was a problem hiding this comment.
This following is a little more idiomatic.
rel := strings.IndexByte(data[start:], '}')
if rel == -1 {
return envPlaceholder{}, fmt.Errorf("unterminated environment placeholder starting at byte %d", dollarIdx)
}
closingBrace := start + rel
| cat > config.yml <<EOF | ||
| listener: | ||
| host: "0.0.0.0" | ||
| port: \${DOLT_TEST_SQLSERVER_PORT:-$SQL_PORT} |
There was a problem hiding this comment.
eek. ok, I guess this answers my previous question. Let's not do this unless someone asks for it. This seems like you could go nuts have have arbitrarily deep nesting which is crazy.
| } | ||
|
|
||
| if ph.hasDefault { | ||
| expandedDef, err := interpolateEnv(ph.def, lookup) |
There was a problem hiding this comment.
This recursive call is kind of strange. Are you saying that the default can be set as an env var too?
port: ${DOLT_PORT:-${DOLT_DEFAULT}}
I would think we'd just have ph.def as a string and use it directly.
There was a problem hiding this comment.
I see the test for this. I think we should keep it simple. people could abuse this is very dumb ways.
| for i := 0; i < len(data); i++ { | ||
| b := data[i] | ||
| if b != '$' { | ||
| out = append(out, data[i]) |
There was a problem hiding this comment.
| out = append(out, data[i]) | |
| out = append(out, b) |
| // Escape sequence: $$ -> $ | ||
| if i+1 < len(data) && data[i+1] == '$' { | ||
| out = append(out, '$') | ||
| i++ | ||
| continue | ||
| } | ||
|
|
||
| // Placeholder: ${...} | ||
| if i+1 >= len(data) || data[i+1] != '{' { | ||
| // Leave stray '$' untouched | ||
| out = append(out, '$') | ||
| continue | ||
| } |
There was a problem hiding this comment.
You could probably save some conditionals by using:
i := strings.Index(s, "$$")
j := strings.Index(s, "${")
up to you.
| if len(b) == 0 { | ||
| return false | ||
| } | ||
| for i := range b { |
There was a problem hiding this comment.
Use a regex!
var envVarRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
func isValidEnvVarName(b []byte) bool {
return envVarRe.Match(b)
}
|
@coffeegoddd DOLT
|
macneale4
left a comment
macneale4
left a comment
There was a problem hiding this comment.
Ship it!
_ _
' \/ '
_ _ <|
\/ __'__ __'__ __'__
/ / / / / /
/\____\ \____\ \____\ _ _
/ ___!___ ___!___ ___!___ \/
// ( ( ( ( ( (
/ / \______\ \______\ \______\
/ / ____!_____ ___!______ ____!_____
/ / / // // /
/ / | || || |
/_____/ \ \\ \\ \
\ \_________\\_________\\_________\
\ | | |
\________!__________!_________!________/
\|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_|_/|
\ _______________ /
^^^%%%^%^^^%^%%^\_"/_)/_)_/_)__)/_)/)/)_)_"_'_"_//)/)/)/)%%%^^^%^^%%%%^
^!!^^"!%%!^^^!^^^!!^^^%%%%%!!!!^^^%%^^^!!%%%%^^^!!!!!!%%%^^^^%^^%%%^^^!
2 participants
Adds support for environment variable interpolation in the
dolt sql-serverconfiguration file (config.yaml).Users can now reference environment variables using the
${VAR}syntax, which will be replaced at runtime with the value of theVARenvironment variable.