-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: Implement custom bootstrapping on startup #85
Conversation
This is related to #64 This adds support for loading from a file,URL or custom implementation |
Pull Request Test Coverage Report for Build 1804982934
💛 - Coveralls |
lib/unleash/bootstrap.rb
Outdated
require 'net/http' | ||
require 'uri' | ||
|
||
module Unleash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to also see a data
bootstrapper out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is now possible after #86 got merged.
The main concern is that the data would need to be serialized in JSON first (to only then be de-serialized).
Having an extra Unleash::Bootstrap::FromData
class doesn't seem like a bad idea though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see Bootstrap as a separate Module, with different classes in it, each providing an implementation of an abstract class.
Note that ideally each class should be in a separate file.
For example please look at https://github.com/Unleash/unleash-client-ruby/blob/main/lib/unleash/strategy/base.rb and https://github.com/Unleash/unleash-client-ruby/blob/main/lib/unleash/strategy/default.rb
If you want/have the time/patience, I can try creating a branch on top of yours to show what I mean, but It could take me a couple of days until i can get around to it.
* feat: make bootstrapping more ruby idiomatic * feat: improved logic, by simplifying. - by instead of passing an object, we pass it as a string, we simplify the code by a lot. - bootstrap reading classes are much simpler now (borderline we should ask ourselves if we still want to bundle them at all, since the responsibility would then just live outside of the SDK) - allow setting bootstrap configuration, and also disabling the toggle fetcher at the same time. Useful when testing. - use as simple test resources as possible. If we don't need it for testing, then we shouldn't bundle it. (as it makes it confusing for whoever comes afterwards) - added tests from the Client class perspective too, so we know that all steps in between work as expected.
This client comes with these classes to load unleash features on startup, before making a request to the Unleash API: | ||
|
||
* Unleash::Bootstrap::FromFile | ||
* Unleash::Bootstrap::FromUri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add examples, including how to set HTTP headers.
|
||
RSpec.describe Unleash::Bootstrap::FromUri do | ||
it 'loads bootstrap toggle correctly from URL' do | ||
bootstrap_file = './spec/unleash/bootstrap-resources/features-v1.json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider in-lining the JSON here. The data is not really re-used in different contexts.
Co-authored-by: Renato Arruda <github@rarruda.org>
PR #85 obsoletes this PR. closing. |
Adds the ability to bootstrap Toggle states from a custom source on load of the client. Currently, there's direct support for loading from a URL and/or loading from a file