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 actors API endpoint #372

Merged
merged 6 commits into from
Aug 21, 2018
Merged

Conversation

kathf
Copy link
Contributor

@kathf kathf commented Aug 2, 2018

Add an endpoint to Flipper::API to check if a feature is enabled for a specific actor.

I'm using Flipper and Flipper::UI in a production web app which is also the backend server for mobile apps. These changes would allow our mobile apps to get data just for the signed-in member, without having to parse all the data from the feature endpoint client-side.

Usage:

get "/feature/my_feature/actor", {'flipper_id' => 'User123'}

@kathf
Copy link
Contributor Author

kathf commented Aug 2, 2018

I'm struggling to fix up the spec failures, but if you're interested in the feature let me know and I will persevere. Thanks

@jnunemaker
Copy link
Collaborator

jnunemaker commented Aug 3, 2018

@kathf thank you so much for contributing!

I remember having a conversation with @AlexWheeler when he first created the API about something like this but I can't find it for the life of me.

Thus far we've went with an API that is smart client, simple server. The server doesn't have logic about enabled-ness. It is only for storage. Adding this might feel like a small change, but it is a big change in approach.

I like the idea of having an easy route you can hit for one actor, especially for read only clients like mobile and clients in other programming languages.

I wonder if the route should be more the inverse of /features, but for a single actor. The route could be /actors/<actor_id> and support a keys param for limiting which features to check (ala /features keys param). This would allow clients to batch read several features at once (say for running every N seconds to lower read throughput or for more detailed caching).

@AlexWheeler @kathf (or anyone else) any thoughts either way on that?

@kathf
Copy link
Contributor Author

kathf commented Aug 5, 2018

@jnunemaker glad to hear you like the idea :)

Yeah I think you're right about the /actors/<actor_id> route with key param for limiting features - it accommodates more use cases and looks neater.

@kathf kathf changed the title Add feature/actor API endpoint Add actors API endpoint Aug 7, 2018
@kathf
Copy link
Contributor Author

kathf commented Aug 7, 2018

Branch updated accordingly.

@AlexWheeler
Copy link
Collaborator

Hey @kathf this is awesome thanks for the PR! As @jnunemaker mentioned definitely a different approach than the API has taken, but I agree with both of you that this will be really useful for the cases mentioned. I also like the idea of an actors/<flipper_id> route with the ability to batch read features. Going to give this a look today

Copy link
Collaborator

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! This is looking great! So close.

If you have more time to take it over the line that would be great, if not, I'd be happy to help. I hate to add burden to contributors like yourself but I have to balance that with ensuring that what is merged is what I'm ready to maintain forever. 😄

I'm only commenting because I'm curious what your thoughts and @AlexWheeler's are. Requesting changes felt to intense for questions. :)

it 'returns all features' do
expected_response = {
'flipper_id' => 'User123',
'features' => [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on making this 'features' => {'my_feature_1' => {'enabled' => true}}. The Hash lookup would make it easier to get at an individual feature. Keeping 'enabled' => true as a hash means we can always add more keys/values if we need. Also, 'features' being a hash means we could always add pagination information there if we ever paginate features due to the sheer number people could have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good call 👍

end
end

context 'when non-existent features are specified' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flipper's default behavior is to not worry about features that don't exist. They are just not enabled. I'm inclined to say this should do the same. Any features passed that don't exist should still be present in the response, but just be enabled => false. That allows us to remove the feature existence check from the action which reduces network calls (like requesting the set of known features) as well. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree.

end
end

def enabled?(feature)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method help us at all? I wonder if we could just inline it since it is only used once and the contents of it are one very simple method call.

@kathf
Copy link
Contributor Author

kathf commented Aug 19, 2018

Hi @jnunemaker I've made all those changes.

I agree with the all of your comments and also I get it about ensuring maintainability for you and the other maintainers. Thanks for writing a great gem!

@jnunemaker
Copy link
Collaborator

@kathf it has been a pleasure working with you. Thanks for the contribution!

@jnunemaker jnunemaker merged commit f43d7b8 into flippercloud:master Aug 21, 2018
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.

3 participants