Config fields defined as slices cannot have commas in item values

[hypnoglow]

Describe the bug

As an example, there is ATHENS_GO_BINARY_ENV_VARS environment variable. When parsing, envconfig splits on comma. Thus, if I want to pass GOPRIVATE=*.corp.example.com,rsc.io/private - this won't work, because value contains comma, which basically results into two wars GOPRIVATE=*.corp.example.com and rsc.io/private=.

Expected behavior

I expect to be able to assign env vars which values can contain commas to Athens config fields like ATHENS_GO_BINARY_ENV_VARS.

Additional context

I guess we might consider providing alternate separator like ;. This way we can pass ATHENS_GO_BINARY_ENV_VARS=GOPRIVATE=*.corp.example.com,rsc.io/private;GOPROXY=direct and this will expand properly into GOPRIVATE=*.corp.example.com,rsc.io/private and GOPROXY=direct. But this also implies that separator ; cannot be used in a value.

[arschles]

@hypnoglow sorry for the delay. I like what you did in gomods/athens-charts#22, but I think we should support both. Instead of using ; as the separator, what do you think of specifying multiple values as a list like. below, and then having Athens decode the list into multiple values?

~/src/gomods/athens(fill-cache) » export GOPRIVATE="[*.corp.example.com,rsc.io/private]"
~/src/gomods/athens(fill-cache) » echo $GOPRIVATE
[*.corp.example.com,rsc.io/private]

[hypnoglow]

If I understood you correctly, you want to provide non-compatible format for standard envs like GOPRIVATE? I neither do think it is a good idea nor see how it solves the problem. ATHENS_GO_BINARY_ENV_VARS=GOPRIVATE=[*.corp.example.com,rsc.io/private];GOPROXY=direct still splits by comma, producing incorrect values.

[jesusvazquez]

Hi

We're having the same issue. Our scenario is that our company has private repositories in many different places, as an example imagine the following list:

• bitbucket.org/company
• gitlab.com/company
• github.com/company
• selfhostedgitsolution.company.com

Now Imagine that we want to set up athens proxy in front of all those repositories, we'd have to define something like:

ATHENS_GO_BINARY_ENV_VARS=GOPRIVATE=bitbucket.org/company,gitlab.com/company,github.com/company,selfhostedgitsolution.company.com;GOPROXY=direct

Unfortunately this is not possible because of the problem explained in the description of the ticket. As a result I can only set GOPRIVATE to one of the sites potentially leaking information about my private repositories in the other sites.

Hope this clarifies a bit the issue.

Regards!

[davidpfarrell]

(this popped up in my subscription to codeTriage so thought I'd take a peek)

I reviewed #1505 and think its the wrong approach - You're just kicking problem down the road to the next person who needs ';' in their value.

Also, what if I want an '=' or a SPACE in my value?

The only scalable approach is to parse the string sequentially and have one or more supported quote/escape patterns.

Scan for first occurrence of '=' - Now you have your KEY and the starting point of your VALUE

In its most-simple form, you could allow for the escaping of the desired pair-separator within the VALUE. In this instance it would mean using \, to indicate that you want an actual , in your VALUE. Again you would have to scan sequentially to distinguish the escaped , from the the pair-separating ,.

Personally, I think the right answer is:

• Use whitespace to separate key-value-pairs and allow quoting of values that need spaces, with support for escaping the quote character within the value. i.e.

ATHENS_GO_BINARY_ENV_VARS=SIMPLE=SimpleValue SPACED="spaced value" QUOTE="\""

A , or ; Would also work as a pair-separator, but SPACE gives you visual separation and flexibility i.e. you could support 1+ spaces between pairs possibly making the pairs easier to distinguish/read visually.

It seems very likely that there are already parsers for these types of strings (although I didn't research it).

Happy to discuss more - Thank you for your time reading my drive-by opinion :)

-DF

[hypnoglow]

@davidpfarrell Valid points.

But, personally, I cannot imagine that someone needs ; in their values:

• For the specific vars mentioned in the issue, I've never seen Go import paths with ;
• What Go-specific env vars support values that may need ;?

Sure, there might be some rare use cases, but we should decide if we want additional complexity to support custom separators or just go with default separator hoping that it will suffice for almost all users.

[davidpfarrell]

But, personally, I cannot imagine that someone needs ; in their values:

Sure, there might be some rare use cases, but we should decide if we want additional complexity to support custom separators or just go with default separator hoping that it will suffice for almost all users.

You realize @ kelseyhightower likely said those exact same words when he decided on the use of , for envconfig? :)

As an aside, one thing I didn't realize is that Athens treats this parsed value as an []string and not map[string]string so nobody (neither Athens nor envconfig) is actually parsing the key=value pairs. (Additionally, envconfig requires : for maps so it couldn't parse it anyway). This leads me to think Athens is trying to be low-touch on the key-value pairs, mostly just treating it as a normal string, so any solution should likely try to honor that if it can.

I also realize that changing the , is a breaking change and should be done with caution.

So if we're okay with a breaking change, then I suppose ; (or maybe &) are quite acceptable.

But if it turns out we are NOT okay with breaking changes, then my updated recommendation would likely be:

Walk the string sequentially, splitting slice entries on , unless its a \, in which case you ignore the \ and leave the , as part of the current slice entry.

Supporting the \, sequence is technically a breaking change, but has little chance of negatively affecting users as anyone who may already be using \, in their value is experiencing bad side effects right now anyway.

NOTE: It does occur to me that we may need to also support \\ in order to allow the rare user who might want to actually end their map value with a \

[edit] spelling, grammar

[marwan-at-work]

I think in this particular case it's okay to introduce a breaking change for a couple of reasons:

1. Athens is not yet v1.x.x and that's precisely why we're not v1 yet...which is to catch these mistakes and make sure they UX is correct. Unfortunately, it will be at the expense of our users but we'll do our best to announce it and provide guides on how to move forward

2. In this case a breaking change seems more valuable than preserving the current flow. This is because the current flow is broken to begin with, there's a 50/50 chance a user is intending to use the , for multiple variables, or within a single variable and therefore we might as well make it correct.

I like the idea of ; and we'll properly mention it in the config documentation that the format will be as such:

ATHENS_GO_BINARY_ENV_VARS="GOPRIVATE=github.com/one/*,github.com/two/*; GOPROXY=proxy.golang.org; GONOSUMDB=github.com/three/*"

And yes, I'm sure there might some rare cases where you need ; -- in this case you can just use our config toml file instead of passing in an env var, so whoever needs it shouldn't be blocked by this :)

If there are any objections to this please let me know.

For those who are running into this issue, know that you already have a couple of work arounds:

1. Use the ATHENS_GONOSUM_PATTERNS if what you're trying to pass is GONOSUMDB or GOPRIVATE

2. Use the config.dev.toml to specify your ATHENS_GO_BINARY_ENV_VARS instead of actually passing env vars :)

I'll try to get the fix merged and released as soon as possible since it's a bit of an important bug.

Thanks everyone!

[hypnoglow]

Amazing! @marwan-at-work thank you for the great work!