Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) #1780
Conversation
gh-yzou
requested review from
adutra,
ashvina,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain,
ebyhr,
ajantha-bhat,
HonahX,
singhpk234 and
pingtimeout
as code owners
| .flatMap(c -> Optional.ofNullable(c.getPolarisCallContext())) | ||
| .map( | ||
| pc -> | ||
| pc.getConfigurationStore() | ||
| .getConfiguration(pc, "ALLOW_WILDCARD_LOCATION", false)) | ||
| ctx -> | ||
| ctx.getPolarisCallContext() | ||
| .getConfigurationStore() | ||
| .getConfiguration(ctx.getRealmContext(), "ALLOW_WILDCARD_LOCATION", false)) |
There was a problem hiding this comment.
I think so, because we need to "undo" the translation from CallContext to PolarisCallContext so that we can recover the RealmContext.
There was a problem hiding this comment.
My bad, didn't notice there was a pc :-)
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
| * @param catalogEntity the catalog to check for an override | ||
| * @param config the configuration to load | ||
| * @return the current value set for the configuration key or null if not set | ||
| * @param <T> the type of the configuration value | ||
| */ | ||
| default <T> @Nonnull T getConfiguration( | ||
| PolarisCallContext ctx, | ||
| RealmContext realmContext, |
There was a problem hiding this comment.
nit: why not keep the old param name?
There was a problem hiding this comment.
i think realmContext is a better parameter name that indicates clearly what the parameter is. "ctx" is very confusing by during the read of code like whether it is for CallContext or PolarisCallContext.
| Assertions.assertEquals("entity-new3", store.getConfiguration(ctx, entity, cfg.apply(3))); | ||
| Assertions.assertEquals("entity-legacy4", store.getConfiguration(ctx, entity, cfg.apply(4))); | ||
| Assertions.assertEquals( | ||
| "test-default1", store.getConfiguration(testRealmContext, entity, cfg.apply(1))); |
There was a problem hiding this comment.
testRealmContext is not relevant here and does not affect the test's behaviour. I'd prefer to keep null.
There was a problem hiding this comment.
sg! I will update this in my part 2 PR.
|
This PR is related to #1775 |
|
Sorry for the auto-merging, hopefully the part 2 can address your comments. cc @gh-yzou. |
getConfiguration should be called per RealmContext instead of per PolarisCallContext. In this PR #1758 we introduced new API to that passes RealmContext, instead. of PolarisCallContext.
This PR refactors all calls except the loadTasks to use the new API with RealmContext, instead of calling with PolarisCallContext.
Follow up PR to update the usage in loadTasks and completely remove the getConfiguration function that uses PolarisCallContext