Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delegate to capture misconfigured feature flags #72

Conversation

josephwoodward
Copy link
Contributor

@josephwoodward josephwoodward commented May 28, 2020

This pull request aims to address #68. I'm interested in your thoughts so any comments then I can update accordingly.

@zhenlan zhenlan requested a review from jimmyca15 May 29, 2020 02:11
@jimmyca15
Copy link
Member

@josephwoodward @zhenlan why do we need a callback instead of a switch like the already existing: IgnoreMissingFeatureFilters? The issue that this is resolving mentions that a missing feature can be an exceptional state. I agree with that. Adding additional functionality via a callback might be a stretch though.

@jimmyca15
Copy link
Member

I do see some comments regarding the capability to set all missing features to true via the callback. I'm not sure what the use case for that would be though? In that case I would expect that a feature really should be configured to true. Depending on any behavior regarding missing features can be dangerous which is exactly what the original issue is mentioning.

@josephwoodward
Copy link
Contributor Author

@jimmyca15

why do we need a callback instead of a switch like the already existing: IgnoreMissingFeatureFilters?

A switch forces your application to either throw an exception or lose any visibility of a missing feature configuration. For instance, if someone were to inadvertently clean up the wrong feature configuration within my Cloud provider of choice then my application would start erroring. This PR proposes adding a callback to enable the consumer to choose how a missing feature configuration is interpreted (log it as an error/warning for instance)

I have no strong opinion on the returned boolean though, personally I'd rather it always returned false.

@jimmyca15
Copy link
Member

@josephwoodward I don't think the callback is the way to go here. Is it true that all calls to the feature manager for a missing feature should behave the same? Perhaps, perhaps not. It's not really standard usage for the library, and I would argue that it is an exceptional state. In that case I would think our best bet would be to stick with standard error propagation via an exception. With that approach, if any logging is needed, then the proper exception could be caught, logged, and rethrown. Or if the application wishes, it could be swallowed.

@josephwoodward
Copy link
Contributor Author

@jimmyca15 I see. At the moment this library does not throw an exception if the configuration is missing so it's not such an issue. With this in mind what are your thoughts on the way forward with the initial issue posed in #68?

@josephwoodward josephwoodward deleted the EnableVisibilityOfMissingConfig branch June 9, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants