RFC: Changing the default plugin env var split character from , to ;

[pat-s]

Currently all plugin vars are concatenated via , and then provided to the plugins:

woodpecker/pipeline/frontend/yaml/compiler/settings/params_test.go

Lines 43 to 56 in fcc57df

	want := map[string]string{
	"PLUGIN_STRING": "stringz",
	"PLUGIN_INT": "1",
	"PLUGIN_FLOAT": "1.2",
	"PLUGIN_BOOL": "true",
	"PLUGIN_SLICE": "1,2,3",
	"PLUGIN_MAP": `{"hello":"world"}`,
	"PLUGIN_COMPLEX": `[{"name":"Jack"},{"name":"Jill"}]`,
	"PLUGIN_COMPLEX2": `{"name":"Jack"}`,
	"PLUGIN_FROM_ADDRESS": "noreply@example.com",
	"PLUGIN_TAG": "next",
	"PLUGIN_TAGS": "next,latest",
	"PLUGIN_MY_SECRET": "FooBar",
	"PLUGIN_UPPERCASE_SECRET": "FooBar",

This creates the following issue:

By default, urfave/cli splits StringSliceFlags on ,, i.e. that a value of PLUGIN_SLICE=1,2,3 will become <argname> 1 <argname>2 <argname>3.
This is fine, works in most cases and is often needed when needing to pass arguments multiple times to a CLI (like in docker for the --build-args argument).
It doesn't work so well when any of the items contain a , on purpose as these will then also be split, whereas they should stay as a single value.

There is a lot of trouble in the buildx plugin for this reason (1, 2, 3) as occasionally values should contain a comma. Right now there is no way to workaround this if the StringSliceFlag value should be kept.

However, urfave/cli provides the option SliceFlagSeparator (default ,). This could be changed to ; and then no splitting at , would happen but only on ;. However, this would require that the incoming env vars from WP also use ; as a separator. I.e., the initial value would then look like PLUGIN_SLICE=1;2;3.

Making this change would solve quite some issues and allow , to be a valid character within StringSliceFlags options. The downside would be that users need to use ; from there onward if they manually supply env var options instead of declaring them as YAML.

[zc-devs]

Or send to plugin entire settings as JSON :)

[lafriks]

But wouldn't this break plugins?

[xoxys]

Please dont. Why not just use escaping? https://github.com/thegeeklab/wp-plugin-go/blob/main/types/stringslice.go

[pat-s]

Ah wow, splitWithEscaping looks were interesting! You should port it to urfave/cli ;) Will try to use that one!

[xoxys]

Looks like someone suggested that already https://codeberg.org/woodpecker-plugins/docker-buildx/issues/70#issuecomment-1331282 and you declined it 🙈 Keep in mind that only works in urfave/cli v2.

[pat-s]

Yes, back then I didn't really realize that we would need it, also because I thought that v3 would solve it. But it doesn't.

Keep in mind that only works in urfave/cli v2.

In the PR it also works with v3. Didn't check in detail why though.
I also found thegeeklab/drone-docker-buildx#142 - oh dear, it seems like this could have been solved much earlier if you (or others) would have pointed me/us there earlier 🙃