Simplify credentials format when bootstrapping

[adutra]

It turns out that only root credentials can be created during bootstrap, since the principal name is hard-coded to PolarisEntityConstants.getRootPrincipalName():

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManagerImpl.java

Line 612 in 390f1fa

PolarisEntityConstants.getRootPrincipalName());

Because of that, the second element in the quadruplet realm,name,id,secret is useless and can be removed.

[eric-maynard]

This is exactly why I don't think this comma-separated format is very good. If this changes in the future, would it be realm,name,id,secret,principal?

I really think we need to change this back to specific env variables or to a structured format rather than make this change.

[adutra]

I'm definitely -1 on specific variables because there is no restriction on which characters can appear in realm ids, client ids and client secrets. These elements should never appear in an environment variable name.

[dimas-b]

I agree. Special chars from realm IDs can make env. var. configuration very difficult in some cases. It becomes flaky and some user will (soon) run into issues... based on my experience with other systems.

That said, I view this change as an on-going cleanup effort not as something set in stone. I think it is perfectly fine to make improvements to bootstrapping that will make these env. variables obsolete, but for now, we need some way to bootstrap and not having to provide superfluous information is an incremental usability improvement.

[eric-maynard]

there is no restriction on which characters can appear in realm ids, client ids and client secrets

How does the current format handle a comma?

[adutra]

How does the current format handle a comma?

It doesn't. I chose comma thinking that nobody would be fool enough to create a realm id or a client id with a comma.

Again, this PR doesn't aim at making things perfect. If you have concerns that people could want a comma in their client ids, feel free to raise a PR and propose an escape mechanism, or a different syntax.

[eric-maynard]

If you have concerns that people could want a comma in their client ids, feel free to raise a PR and propose an escape mechanism, or a different syntax.

I did propose a different syntax here and that PR merged anyway

[adutra]

I did propose a different syntax here and that PR merged anyway

And I did explain right below why I thought it wasn't a great idea. And you even agreed with my explanation.

I'm sorry but I won't change my mind: JSON imho is not a good fit here. If you think otherwise, open a PR for it, gather consensus, and then merge it.

[eric-maynard]

I did not agree; when I mentioned the existing syntax, I was referring the the syntax which existed in main at the time of writing -- not the new one you were proposing.

Even if you interpreted that comment as agreeing with the proposed changes, I agreed contingent on a more "ergonomic" command-line syntax being introduced... which didn't happen.

[dimas-b]

#633 has been merged. That's the current state of the code. We should certainly keep the lessons learned from the related discussion in mind for the future.

Nonetheless, why not simplify the values of the env. variable that is already in effect (this PR), while future bootstrap improvements are still kind of up in the air? This PR may not be perfect, but IMHO in its current form it is still a net improvement to the codebase.

[adutra]

Are we OK to merge this?

[eric-maynard]

No, I was not okay to merge this. Hence the -1