Flag variant attachment

[kevin-ip]

Extending variant to support attachment

• Extend flipt.proto to support attachment in variant
• Extend DB with the new column
• Extend UI to support the new field

closes #188

[markphelps]

Looking good so far!

However, I think it's missing the attachment field in the EvaluationResponse to be useful.

I think you'll need to add the attachment column in this SQL statement and also add VariantAttachment and set the value to the attachment of the variant that matches during evaluation

See these screenshots where there is no attachment that comes back for the variant when there should be:

[kevin-ip]

However, I think it's missing the attachment field in the EvaluationResponse to be useful.

Thanks for the promptly review. I've updated it. Please let me know if I missed anything. Thanks

[markphelps]

Looks awesome @kevin-ip ! Thanks for this. I'll check it out more thoroughly tonight and likely merge it and cut a release this week

[markphelps]

this test failure also brings up a good question, should we strip whitespace from the JSON on the server side? or UI side or both?

[kevin-ip]

this test failure also brings up a good question, should we strip whitespace from the JSON on the server side? or UI side or both?

I like it on the server side as we don't have to do it twice (server + UI). WDYT?

[kevin-ip]

@markphelps Just took a look and it seems the DB perform some beautification on the json.
Trying to pass an attachment with extra spaces after colon:

Think we couple options:

1. perform compaction when reading the attachment from the DB
2. Don't do anything and let the DB decides.

I like option 1 for the app will have consist result across different DBs.

[markphelps]

@kevin-ip yeah I agree that option 1 is the cleanest approach

[jalaziz]

If we're comfortable saying the attachment must always be JSON, then it might be nice to have the attachment imported from / exported as YAML. It would make for more human readable/editable exports.

[markphelps]

If we're comfortable saying the attachment must always be JSON, then it might be nice to have the attachment imported from / exported as YAML. It would make for more human readable/editable exports.

@jalaziz good idea. I'll add this as a follow up PR before creating a release, I don't want to block the merging of this PR any longer than necessary

[markphelps]

@allcontributors please add @kevin-ip for code

[allcontributors]

@markphelps

I've put up a pull request to add @kevin-ip! 🎉

[markphelps]

@allcontributors please add @amayvs for code

[allcontributors]

@markphelps

@amayvs already contributed before to code

[markphelps]

This is an amazing addition @kevin-ip @amayvs ! Thank you so much for contributing. I'll create a release before the weekend. Also thank you @jalaziz for helping review