Fixes #2944, #2947 - Read experiment variation values from envvars

[miketaylr]

This feels maybe slightly hacky -- we can't safely store a tuple in an environment variable, so it build one up from something like 0 20. Open to other ideas.

And add tests.

r? @johngian @karlcow

[miketaylr]

Oops, wrong bug number. :) Will fix the commits.

[karlcow]

some modifications and comments.
Thanks a lot @miketaylr for working on this.

[johngian]

I was thinking of a different approach that
a) Would work for other experiments as well
b) Has less hard-coded values

can be something like that: johngian@0662bf1

The reasons that I came up with this is that if something goes very bad and we need to override the A/B testing cookies (in our case exp), this would require a code deployment. Where, if we manage the whole A/B config definition we can just change the value there.

Other than that, this solution also looks OK.

[miketaylr]

I was thinking of a different approach

Looks good to me -- so the idea is to export a JSON string as an env var? I've never done that, but
it makes sense.

My only concern is writing out complex JSON by hand is error-prone, maybe it's useful for us to write a tiny script that will echo it out for us?

Or do you have any other suggestions?

[miketaylr]

^^ @johngian

[johngian]

@miketaylr I haven't really thought about that since its more a once-off thing (writing the config i mean) and if you are really careful and pass it through a JSON linter you are probably good to go. It feels like writing a script to generate a JSON for the config its too much for our case.

[miketaylr]

@miketaylr I haven't really thought about that since its more a once-off thing (writing the config i mean) and if you are really careful and pass it through a JSON linter you are probably good to go. It feels like writing a script to generate a JSON for the config its too much for our case.

thanks -- i think let's just go with this approach and i'll file a follow-up to simplify things via JSON parsing (more simple implementation, slightly higher risk). i'll work on addressing feedback.

[karlcow]

@miketaylr I haven't really thought about that since its more a once-off thing (writing the config i mean) and if you are really careful and pass it through a JSON linter you are probably good to go. It feels like writing a script to generate a JSON for the config its too much for our case.

thanks -- i think let's just go with this approach and i'll file a follow-up to simplify things via JSON parsing (more simple implementation, slightly higher risk). i'll work on addressing feedback.

the issue with a JSON file is that we are back to square one. As something which is dependent of a deploy (If I got what was proposed)

[miketaylr]

the issue with a JSON file is that we are back to square one. As something which is dependent of a deploy (If I got what was proposed)

I think the suggestion is to use a JSON string, not a file. So you export a var that contains JSON, rather than read it from disk. But I guarantee I will misquote the JSON and everything will fail. 😄

[miketaylr]

I think I addressed all feedback, r? @karlcow

Right now, if get_variation_from_env raises because something is wacky, it will print to stdout. So we'll have to make sure we don't see any weird messages when restarting the app after updating values.

[miketaylr]

OK, not 100% sure I love this implementation, but it works and the tests pass.

(It doesn't feel very DRY, but I'm not sure I care. It would have been nice to be able to read the VARIATIONS and DEFAULTS dicts from the config during testing, rather than have to pass them in to be testable, but I don't think it's possible to refer to the config object inside of get_variation because at the point it gets called, the config object hasn't even been created.)

r? @karlcow

[miketaylr]

(before merging, we should probably squash this into a single commit -- i'm happy to do so)

[miketaylr]

This will also fix #2947 because by default the variables won't be set, and it will only serve v1 of the form (which tests rely on).

[karlcow]

Thanks @miketaylr
yeah the code seems heavy, but I'm not sure I have a better proposal right now.

[karlcow]

ok squashed committed. Let's run CircleCi and wait a bit.

[karlcow]

merged