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

Rollout Adapter for importing rollout data into Flipper #319

Merged
merged 1 commit into from
Jan 4, 2018

Conversation

AlexWheeler
Copy link
Collaborator

Addresses #308

Adds an adapter to import Rollout data into Flipper

Rollout supports the equivalent to Flipper's

  • groups
  • actors
  • percentage_of_actors
  • boolean (sort of...via 100% percentage enabled)

Rollout does not support

  • percentage_of_time

Importing only requires features, get to be implemented.

Questions/Notes:

  • for the methods the adapter doesn't implement I'm thinking of raising an error similar to the read only adapter to make it clear that this adapter is only for importing since rollout doesn't have feature parity

  • I'm testing ["can have one feature imported"](https://github.com/jnunemaker/flipper/compare/rollout-adapter?expand=1#diff-ce5ff96d129103e0d6d194b6be39790dR59) type stuff where its not really testing the rollout adapter its really testing adapter.import, but I think it does give some assurance that things are being imported correctly. Alternatively we could just assume import works given that get, features` work which are also unit tested, but I do like the more thorough test.

  • this will only work with rollout ~> 2.0 because the Rollout public api changed from 1 -> 2 and this is reflected in the gemspec dependency.

  • Rollout 2.3 introduced casting percentages to_f before saving. For flipper users using Flipper < 0.11 (before Flipper supported decimal percentages) the values between Rollout (decimal) and Flipper(integer) would be different, although I don't think this is a big deal?

  • users importing Rollout data would need to reregister their groups since those are an evaluated block in memory

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.

SWEEEEEEEET. I love this so much, mostly because it shows the flexibility and removes a tiny road block for switching if someone is interested. I really appreciate that you took the time to both implement this and explain all the trade-offs. Well done! 👏

for the methods the adapter doesn't implement I'm thinking of raising an error similar to the read only adapter to make it clear that this adapter is only for importing since rollout doesn't have feature parity

👍

I do like the more thorough test.

I like it too.

this will only work with rollout ~> 2.0 because the Rollout public api changed from 1 -> 2 and this is reflected in the gemspec dependency.

Something we can document easily enough.

Rollout 2.3 introduced casting percentages to_f before saving. For flipper users using Flipper < 0.11 (before Flipper supported decimal percentages) the values between Rollout (decimal) and Flipper(integer) would be different, although I don't think this is a big deal?

Probably not a big deal. We can add it as a caveat on the rollout docs and tell people to make sure they are running >= 0.11

users importing Rollout data would need to reregister their groups since those are an evaluated block in memory

Yep, great point. Seems totally fine to document this as well.

@jnunemaker
Copy link
Collaborator

I can drop a new release after this merges or wait and we can put some docs in. I am leaning toward 0.11.1 instead of 0.12 since this would just be an addition. @AlexWheeler What do you think?

@AlexWheeler AlexWheeler force-pushed the rollout-adapter branch 2 times, most recently from 5b61dae to 41801e3 Compare January 4, 2018 00:49
@AlexWheeler
Copy link
Collaborator Author

@jnunemaker I'll open another issue to document this and hopefully get to that this week

I think its all good to release new version then get docs up. I'd say 0.12 would be the correct semver since as you mention its an addition and not a bugfix, but 0.11.1 is cool too.

@AlexWheeler AlexWheeler merged commit 819e8ab into master Jan 4, 2018
@AlexWheeler AlexWheeler deleted the rollout-adapter branch January 4, 2018 03:12
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.

2 participants