(breaking) Prefix all envbuilder environment variables with ENVBUILDER_

[johnstcn]

For consistency with coder/coder, we should prefix all environment variables consumed by envbuilder with the string ENVBUILDER_.

This will be a breaking change and must be done before any guarantee of future stability.

[bpmct]

Totally makes sense. If we were to support #127, how do you imagine that would work? ENVBUILDER_KANIKO_*?

[johnstcn]

Envbuilder currently imports kaniko as a library, so I don't believe this would impact how kaniko reads its environment variables.

[bpmct]

So if we wanted to support all (or most) kaniko options, we would rewrite instead of passing through?

[johnstcn]

KANIKO_DIR is just looked up on-demand at present so we do not need to do anything for that env.

Other kaniko options are checked in its root command (for example, KANIKO_REGISTRY_MIRROR, KANIKO_REGISTRY_MAP and KANIKO_NO_PUSH). We currently have to check for the presence of this environment variable and pass it in as required.

I don't think we would want to prefix these with ENVBUILDER_ in either case.

[BrunoQuaresma]

After discussing this on Coconut standup we want to:

• Add env variables prefix option to the coder/serpent and after support this on the envbuilder
• The "prefix env variable" should still support the env variables without the prefix so it does not break old consumers

[matifali]

The "prefix env variable" should still support the env variables without the prefix so it does not break old consumers

We should only support old format until we do 1.0 that too with a deprecation message that it is going to be removed in release 1.0.

[BrunoQuaresma]

Thinking more about how we want to make it, I wonder if we could make coder/serpent to accept Env as an []string instead. IMO, it makes the code more clear and direct about the usage. Another option would be to Env as any and accept string or string[].

const prefix = "CODER_"

return serpent.OptionSet{
  {
    Flag:  "setup-script",
    Env:   []string{"SETUP_SCRIPT", prefix + "SETUP_SCRIPT"},
    Value: serpent.StringOf(&o.SetupScript),
    Description: "...",
  },
}

Wdyt?

[matifali]

Thinking more about how we want to make it, I wonder if we could make coder/serpent to accept Env as an []string instead. IMO, it makes the code more clear and direct about the usage. Another option would be to Env as any and accept string or string[].

const prefix = "CODER_"

return serpent.OptionSet{
  {
    Flag:  "setup-script",
    Env:   []string{"SETUP_SCRIPT", prefix + "SETUP_SCRIPT"},
    Value: serpent.StringOf(&o.SetupScript),
    Description: "...",
  },
}

Wdyt?

What if we add new fields for deprecated environment DeprecatedEnv and DeprecationMessage? And allow coder/serpent to still accept them but show a deprecation message.
It can make it easier to remove the deprecated values later.

[BrunoQuaresma]

@matifali I think this can be interesting. Let's see what other folks think about cc.: @mafredri @mtojek

[mafredri]

I think you both are on to something, but I'd like to know what @ammario thinks about introducing breaking changes to the API.

Another way to achieve this could be by introducing aliases, perhaps something like this:

type OptionAlias struct {
	Flag          string `json:"flag,omitempty"`
	FlagShorthand string `json:"flag_shorthand,omitempty"`
	Env           string `json:"env,omitempty"`
	Deprecated	  string `json:"deprecated,omitempty"`
}

// Option is a configuration option for a CLI application.
type Option struct {
	Aliases []OptionAlias `json:"aliases,omitempty"`
	// ...
}

// Usage:
{
	// ...
	Env: "CODER_FOOBAR",
	Alias: []serpent.OptionAlias{
		{Env: "FOOBAR", Deprecated: "Use CODER_FOOBAR instead."},
	},
},

We could then create a convenience function in envbuilder that adds the deprecated alias to an option.

There already exists UseInstead that can serve a similar purpose, but I feel that solves the issue in reverse.

Ultimately, I love the idea of being able to define multiple envs for a flag. We have such use-cases in coder/coder too, for instance.

[ammario]

Re prefixing all environment variables, there is ParseEnviron for this very reason.

In the meantime if envbuilder needs to support multiple environment variables, we can use UseInstead or even manipulate the Invocation.Environ before its ran.

We should take any conversation about changing the serpent API to that repo to keep this tracker clean. I created an issue here for aliases.