feat: FlagsmithProvider #126
Conversation
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
…coded JSON array Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
…faultValue Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
ChrisLightfootWild
force-pushed
the
provider/flagsmith
branch
from
43e7651 to
e0bf3b6
Compare
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
There was a problem hiding this comment.
Is it possible to detect if a flag isn't found?
There was a problem hiding this comment.
At the moment, the Flagsmith client throws a generic FlagsmithClientError in this situation, but I don't see any reason not to use this opportunity to improve the Flagsmith SDK too and extend that exception for a specific FlagsmithFlagNotFound error, similar to how we do it in the python client here.
There was a problem hiding this comment.
Great, so this could be implemented once the underlying SDK supports the feature 👍
There was a problem hiding this comment.
At the moment, the Flagsmith client throws a generic FlagsmithClientError in this situation, but I don't see any reason not to use this opportunity to improve the Flagsmith SDK too and extend that exception for a specific FlagsmithFlagNotFound error, similar to how we do it in the python client here.
| if ($flag->getEnabled()) { | ||
| /** @var array<array-key, mixed>|bool|DateTime|float|int|string|null $value */ | ||
| $value = $flag->getValue(); | ||
|
|
||
| $builder->withValue($value); | ||
| } else { |
There was a problem hiding this comment.
In other providers (e.g. this logic in the python client) we only check this value before returning the value based on a configuration argument passed to the initialisation of the provider.
I'm not against handling this differently here, but just though I'd highlight it.
There was a problem hiding this comment.
@matthewelwell should that be a concern of the Flagsmith SDK? If an integrator was using the SDK directly, should they also replicate that logic?
Not against changing it, just trying to clarify for my own understanding.
There was a problem hiding this comment.
Well, I think my point here is that we're implicitly adding that logic to the provider without a way to change it, right? If someone wants to define a flag in flagsmith to have a value, but remain disabled then there is no way for them to retrieve that value using this implementation of the OpenFeature provider.
In fairness, it's probably not a massive use case, and can be changed in the future if needed, so it's not an important point. I think I'm mostly just posing the question to maintain consistency across our providers, but it's not a hill I'm willing to die on, so if you disagree then I'll go ahead and approve with this behaviour (notwithstanding the other comments).
There was a problem hiding this comment.
@matthewelwell I will defer to your expertise here, I was just wanting to better understand some of the rationale.
I'm going to align with the python provider now, thanks.
| public function resolveStringValue(string $flagKey, string $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } | ||
|
|
||
| public function resolveIntegerValue(string $flagKey, int $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } | ||
|
|
||
| public function resolveFloatValue(string $flagKey, float $defaultValue, ?EvaluationContext $context = null): ResolutionDetails | ||
| { | ||
| return $this->resolve($flagKey, $defaultValue, $context); | ||
| } |
There was a problem hiding this comment.
Since the value from the flagsmith SDK can have different types, shouldn't we do some string casting here (or in the resolve method perhaps?
There was a problem hiding this comment.
Good point, I will investigate this further.
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
Signed-off-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
This PR
open-feature/flagsmith-providerOpenFeature\interfaces\provider\Providerinterface for Flagsmithflagsmith/flagsmith-php-clientNotes
Follow-up Tasks
How to test
Run the tests with:
You should (hopefully) see the below output:
