Add missing "overrides" to realm-override config examples & tests

[RichardLiu2001]

The existing examples of setting realm-override configurations are missing the "overrides" json key:

public interface FeaturesConfiguration {

  interface RealmOverrides {
    Map<String, String> overrides();
  }

So, instead of
polaris.features.realm-overrides."my-realm"."feature"=true
it should be
polaris.features.realm-overrides."my-realm".overrides."feature"=true

Thinking a bit more about where the issue is.

[adutra]

@RichardLiu2001 sorry I approved too soon :-)

There are two issues here:

1. You spotted a bug in the configmap generation where we were using polaris.config instead of polaris.features. I think it's legit to fix that.
2. You spotted that the word .overrides. is missing in polaris.features.realm-overrides."my-realm".overrides."feature"=true – true, but I think the error is in FeaturesConfiguration.RealmOverrides: the RealmOverrides#overrides method should have been annotated with @WithParentName.

For 2, it's a regression introduced by b93e97b. @eric-maynard was your change intentional, that is, you want the word .overrides. to appear in the configuration, or is it OK to revert the changes to QuarkusFeaturesConfiguration?

[eric-maynard]

nice catch!

[singhpk234]

Nice catch !

[adutra]

I'm not sure this PR should be merged as is. If for nothing else, the default application.properties is not aligned with the Helm chart, for the reasons I mentioned in my previous comment.

[eric-maynard]

@adutra good point on (2) -- I would be okay changing it back to remove overrides... to be honest, I still find the post-Quarkus method of specifying a config's value really arcane. We went from e.g. featureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES to polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES". Why is it not just featureConfiguration.X = Y?

[adutra]

@adutra good point on (2) -- I would be okay changing it back to remove overrides... to be honest, I still find the post-Quarkus method of specifying a config's value really arcane. We went from e.g. featureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES to polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES". Why is it not just featureConfiguration.X = Y?

I'm all in to try and reduce the verbosity of the configuration options. But, I think it's also nice to always prefix our own options with "polaris." What I think we should try is to remove the "defaults." part. It should be possible imho by annotating the corresponding method with @WithParentName.

[gh-yzou]

actually, never mind, this seems adding another key at the nest level, do we wan to add another key (which kind of changes how the config of overrides look like before), or revert the change before? cc @adutra @eric-maynard

[eric-maynard]

If it works with @WithParentName, I think we should take that approach. From a behavior perspective, IMO we should try to revert back to the existing behavior. Ideally before 1.0.

[gh-yzou]

If we want to fix to keep the old behavior, then the only doc update is "config" -> "features" change

[eric-maynard]

@adutra I'm hoping to fix this in #1572

... I think the error is in FeaturesConfiguration.RealmOverrides: the RealmOverrides#overrides method should have been annotated with @WithParentName ... is it OK to revert the changes to QuarkusFeaturesConfiguration?

Can you clarify whether FeaturesConfiguration.realmOverrides or QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation? Per my understanding it's the latter but just want to confirm.

edit: from reading the docs, it seems like WithParentName should be on QuarkusFeaturesConfiguration.realmOverrides?

[gh-yzou]

@eric-maynard In my other PR, i have a test to help testing he behavior https://github.com/apache/polaris/pull/1505/files#diff-d050121a0a07b1b4c8c7df231afbb927fe6cb04b2d4060a01fdbda96e9c568dfR45, you can probably add similar test in your fix to make sure the behavior is correct. However, based on my previous testing, it is QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation.
i have one quick question, is there any fix needed for QuarkusBehaviorChangesConfiguration ?

[adutra]

Can you clarify whether FeaturesConfiguration.realmOverrides or QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation? Per my understanding it's the latter but just want to confirm.

@eric-maynard the previous state before b93e97b would be to annotate only QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides.

You can try to also annotate FeaturesConfiguration.realmOverrides but 1) that wouldn't match the previous state and 2) I'm not sure it would work because there is another map at the same level: FeaturesConfiguration.defaults.