feat: custom delimiters for environment variables

[karthikb351]

Adds support for custom delimiters for environment variables.

Currently we are naively just splitting the string on , via str.split(","), which also splits commas that might be inside quotes.

For example

env_vars: KEY1="val1.1,val1.2",KEY2=val2

gets split as

[ 'KEY1="val1.1', 'val1.2"', 'KEY2=val2' ]

This PR adds the option to set a custom delimiter via the env_vars_delimiter key.

Added a test cases for a common example of where this would be useful - passing JSON stringified objects as environmental variables.

[karthikb351]

@averikitsch looks like I'm failing the lint check, I'll update this PR.

I'm also not entirely sure if the regex is the right way to go about this or if there's some simpler way.

[averikitsch]

@averikitsch looks like I'm failing the lint check, I'll update this PR.

I'm also not entirely sure if the regex is the right way to go about this or if there's some simpler way.

You can run npm run fixlint. Regex is a good solution here IMO. Though can you add a few more test cases for the method parseKVPairs?

[bharathkkb]

@averikitsch I think another way could be to allow users to specify the an alternate delimiter (defaulting to ,). This aligns well with gcloud. WDYT?

[averikitsch]

@bharathkkb I do like the consistency with gcloud, but that might be over engineering for what most people need. Ignoring commas in quotes seems like an easy solution, but do we anticipate it breaking in any way? Does JSON break this ie KEY="{"key1":"value1","key2":"value2"}"?

[bharathkkb]

@averikitsch

I do like the consistency with gcloud, but that might be over engineering for what most people need

Yes agreed, but with a default to , most folks wont need to touch this (like most people wont do gcloud functions deploy foo --set-env-vars "^:^a,b:c,d" . I also think that is more maintainable than maintaining/testing regex like

deploy-cloud-functions/src/cloudFunction.ts

Lines 166 to 180 in 5b0cfcc

	/**
	* Regex to split on ',' while ignoring commas in double quotes
	* /, // Match a `,`
	* (?= // Positive lookahead after the `,`
	* (?: // Not capturing group since we don't actually want to extract the values
	* [^\"]* // Any number of non `"` characters
	* \" // Match a `"`
	* [^\"] // Any number of non `"` characters
	* *\" // Match a `"`
	* )* // Capture as many times as needed
	* [^\"] // End with any number of non `"` characters
	* *$) // Ensure we are at the end of the line
	* /g // Match all
	*/
	const valuePairs = values.split(/,(?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)/g);

[karthikb351]

@bharathkkb @averikitsch I've refactored this a bit, will try and summarize the changes here

• I moved the parsing envVars logic to a unit test since the current function was protected. It was a non-public function was purely functional.
• I added/refactored a bunch of explicit unit test cases for the different inputs for env vars (quoted, unquoted, multiple=)
• I also moved the parseEnvVarsFile into util and moved the test cases there.
• Resolved the merge conflicts introduced when feat: Merge EnvVars and EnVarsFile (#134) #139 landed. (I rebased the branch and force pushed a cleaner commit history)

[bharathkkb]

@karthikb351 any thoughts on #137 (comment)

[karthikb351]

@karthikb351 any thoughts on #137 (comment)

Not sure if this is overkill. The most common issue with requiring a different delimiter is because , might be part of VALUE, but quotes solves that anyway, why would you need a different delimiter if it's quoted?

[bharathkkb]

@karthikb351 after some thinking I think we should go with the adding an optional env_vars_delimiter defaulting to , because

• complexities with values that use quotes as part of the value, I am thinking json values like @averikitsch pointed out KEY="{"foo":"v1,v2","bar":"v3"}",KEY2=FOO
• if a user were to use double quoted scalars, quotes will need to be escaped or switch to plain/single quoted scalars - env_vars: "KEY1="VALUE1,VALUE2",KEY2=VALUE2" would not be valid yaml
• regex is generally harder to debug/maintain when there are any edge cases

[karthikb351]

@bharathkkb fair points, especially with edge cases like KEY="{"foo":"v1,v2","bar":"v3"}",KEY2=FOO, where the regex I'm using will fail.

If you use a custom delimiter, you are going to end up with a similar problem because the delimiter could exist inside the value. You'll have to build some way to allow for escaping the delimiter.

This entire pipeline is so complicated, ideally you'd want github actions to support array inputs, so you could do

env_vars:
  - KEY1=VAL1
  - KEY2=VAL2

But GitHub Actions core doesn't support this because they parse every input as a single env var, so they will have a similar issue of having to join the array into a string and split it again.

Maybe a completely different approach would be to do something like

env_vars: |
  KEY1=VAL1
  KEY2=VAL2

and then split on newline?

This is how GitHub's actions/cache:src/utils/actionUtils.ts#L56-L65 does it.

[bharathkkb]

If you use a custom delimiter, you are going to end up with a similar problem because the delimiter could exist inside the value.

@karthikb351 I am proposing we expose the env_vars_delimiter which should let the user select the right delimiter for their use case. This would also support on splitting on newline if they provide env_vars_delimiter: '\n' .

We can also evaluate switching to\n as default before 1.0 but for now we can default to , if no explicit env_vars_delimiter is set to prevent any breakage.

[karthikb351]

If you use a custom delimiter, you are going to end up with a similar problem because the delimiter could exist inside the value.

@karthikb351 I am proposing we expose the env_vars_delimiter which should let the user select the right delimiter for their use case. This would also support on splitting on newline if they provide env_vars_delimiter: '\n' .

We can also evaluate switching to\n as default before 1.0 but for now we can default to , if no explicit env_vars_delimiter is set to prevent any breakage.

Makes sense. I'll update the PR.

[karthikb351]

@bharathkkb @averikitsch Added the env_vars_delimiter opts and updated the PR description. I need to update the tests with a broader set of cases, and also README, docs, etc.

Any other pointers in the meantime?

[bharathkkb]

Thanks @karthikb351 this is looking good!

[bharathkkb]

Suggested change

	envVarsDelimiter?: string;
	envVarsDelimiter:string = ',';

[bharathkkb]

@karthikb351 gentle ping if you are still planning to wrap this up.

[karthikb351]

@bharathkkb yup! Will get around to it this weekend, been a busy few days.