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

Projects
None yet
2 participants
@AlexWheeler
Collaborator

AlexWheeler commented Dec 22, 2017

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

@jnunemaker

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

This comment has been minimized.

Owner

jnunemaker commented Dec 23, 2017

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 Jan 4, 2018

@AlexWheeler

This comment has been minimized.

Collaborator

AlexWheeler commented Jan 4, 2018

@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 force-pushed the rollout-adapter branch from 41801e3 to bdc358f Jan 4, 2018

@AlexWheeler AlexWheeler force-pushed the rollout-adapter branch from bdc358f to 4db94bd Jan 4, 2018

@AlexWheeler AlexWheeler merged commit 819e8ab into master Jan 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@AlexWheeler AlexWheeler deleted the rollout-adapter branch Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment