Simplify the environment variables needed for credential bootstrapping #633
Conversation
adutra
requested review from
ashvina,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
flyrain and
ebyhr
as code owners
| ```bash | ||
| env POLARIS_BOOTSTRAP_DEFAULT-REALM_ROOT_CLIENT_ID=my-client-id POLARIS_BOOTSTRAP_DEFAULT-REALM_ROOT_CLIENT_SECRET=my-client-secret <bootstrap command> | ||
| ``` | ||
| export POLARIS_BOOTSTRAP_CREDENTIALS=my_realm,root,my-client-id,my-client-secret;my_realm2,root,my-client-id2,my-client-secret2 |
There was a problem hiding this comment.
This syntax feels a bit obtuse... I would almost prefer a JSON blob or something
There was a problem hiding this comment.
I thought about that too but shoveling json in an environment variable inside a shell script is calling for trouble with double-quotes and escape chars. So I thought this plain text format was more appropriate. Wdyt?
There was a problem hiding this comment.
That makes sense. In that case maybe the existing syntax is okay, and for a more "ergonomic" syntax we can consider arguments to the bootstrap command itself? Perhaps a JSON payload there.
There was a problem hiding this comment.
That's exactly my plan in #605 – for now, it needs the environment variable, but I will introduce proper arguments to the bootstrap command.
There was a problem hiding this comment.
from my POV the complexity of the value is justified by not having to deal with - in the env. var. names (which is part of the default-realm name)
| @Test | ||
| void nullString() { | ||
| PolarisCredentialsBootstrap credentials = PolarisCredentialsBootstrap.fromString(null); | ||
| assertThat(credentials.credentials).isEmpty(); |
There was a problem hiding this comment.
nit: why not make a package-private "parse" method visible to tests, but keep the field private? I think it'd provide better encapsulation.
I know there has been some back and forth on this topic, so apologies to bring it up again.
I would like to propose a simplification of the environment variables needed to bootstrap credentials, for a few reasons:
Instead I'm proposing to use just one system property or environment variable that contains all the data we need.