WIP: integrating Yggdrasil #223
Conversation
| @@ -76,6 +76,7 @@ public ClientFeaturesResponse fetchFeatures() throws UnleashException { | |||
| FeatureCollection features = | |||
| JsonFeatureParser.fromJson( | |||
| Objects.requireNonNull(response.body()).charStream()); | |||
| // TODO is this being used? Or HttpFeatureFetcher took over? | |||
There was a problem hiding this comment.
Can we delete OkHttpFeatureFetcher? I couldn't find valid references to it
| } catch (YggdrasilInvalidInputException e) { | ||
| LOG.error("Failed to take state", e); | ||
| } |
There was a problem hiding this comment.
@sighphyre do you think this is ok?
| return isEnabled(toggleName, contextProvider.getContext(), defaultSetting); | ||
| try { | ||
| UnleashContext enhancedContext = contextProvider.getContext().applyStaticFields(config); | ||
| return Optional.ofNullable(unleashEngine.isEnabled(toggleName, adapt(enhancedContext))).orElse(defaultSetting); |
There was a problem hiding this comment.
UnleashEngine can return null when calling isEnabled and the SDK needs to handle this situation and provide a default value if present
| // PR-comment: constraints are no longer managed by the SDK but by Yggdrasil, so we removed the third parameter | ||
| verify(fallback).isEnabled(any(), any()); |
There was a problem hiding this comment.
I just wanted to double/triple check that it's ok to drop constraints parameters here
There was a problem hiding this comment.
@chriswk maybe you know, but this is a public interface we want to get rid of:
The constraints are evaluated by Yggdrasil, and I believe this is just to do the evaluation in the SDK, but no one should ever extend this, but it's possible. So, I wonder how should we move this forward, we have the opportunity to make a hard-breaking change.
gastonfournier
force-pushed
the
yggdrasil-engine
branch
from
c7cc451 to
ba5370c
Compare
| try { | ||
| VariantDef variantResponse = this.unleashEngine.getVariant(toggleName, adapt(enhancedContext)); | ||
| if (variantResponse != null) { | ||
| return new FeatureEvaluationResult(variantResponse.isEnabled(), | ||
| new Variant(variantResponse.getName(), adapt(variantResponse.getPayload()), variantResponse.isEnabled())); | ||
| } |
There was a problem hiding this comment.
Should we always try to read variants first?
gastonfournier
force-pushed
the
yggdrasil-engine
branch
from
128b5fe to
48811ed
Compare
gastonfournier
force-pushed
the
yggdrasil-engine
branch
3 times, most recently
from
255a577 to
10387e1
Compare
gastonfournier
force-pushed
the
yggdrasil-engine
branch
from
10387e1 to
3862610
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is a WIP PR with the idea of gathering early feedback. Note that this is intended to "Work on my machine"