-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: support yaml evaluator #206
feat: support yaml evaluator #206
Conversation
a51f64d
to
19a66fb
Compare
This PR is WIP. I will request review once I am done by mentioning the reviewer in a comment on this issue. |
Hey @vadasambar ! Since it tends to be trivial to convert yaml <=> json, I would suggest somehow approaching the problem with that in mind. You can leverage the existing JSON evaluator's logic and state storage and just change the parsing, I think. Perhaps composition could help here? |
e832622
to
cef8463
Compare
1afef7d
to
07e75c5
Compare
62ef1b0
to
3daacc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also update the configuration documentation?
https://github.com/open-feature/flagd/blob/main/docs/configuration.md
@beeme1mr slipped my mind. I will do it. Thanks for pointing it out. 🙏 |
Hi @vadasambar, could you please fix the conflicts and resolve the issue with the test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with #206 (comment)
Thanks again for all your effort, @vadasambar
Makes sense to me. 👍 |
Thank you for creating the issue 🙏 |
Not at all. Thank you for the feedback and review 🙏 |
I think we need review from @AlexsJones to merge this PR (not sure if we want to keep it open until #240 is done) |
I think we need review from @AlexsJones |
I don't think we need to include that in this PR, it seems like something we can roll in later, but feel free to start working on it if you are interested. |
Signed-off-by: Suraj Banakar <[email protected]> feat: wip try replacing json evaluator with a generic file type evaluator - TODO: - still need a way to make the json_evaluator_model more composable - replace instances of `json` with the `MUaler` interface - implement `MUaler` interface for yaml and json Signed-off-by: Suraj Banakar <[email protected]> feat: wip pass `yaml` as filetype to `FilePathSync` - check `yaml.Unmarshal` not able to `Unmarshal` to `json.rawMessage` which is just `[]byte` with `MarshalJSON` and `UnmarshalJSON` - `FileTypeEvaluator` didn't turn out to be such a good idea because we are reliant on `gojsonschema` to test json schema - add `example_flags.yaml` file based on `examples_flags.json` refactor: remove traces of `FileTypeEvaluator` Signed-off-by: Suraj Banakar <[email protected]> feat: wip try yaml->map->json conversion - the code compiles and the converted json seems to look good but flagd throws error - need to look into this ^ Signed-off-by: Suraj Banakar <[email protected]> feat: wip remove yaml annotationt and redundant code - yaml config -> json config -> json.Unmarshal (<- error here) Signed-off-by: Suraj Banakar <[email protected]> feat: wip add indentations in the json converted from yaml - because the evaluator transposer function doesn't understand json without indentations well - needs more testing Signed-off-by: Suraj Banakar <[email protected]> feat: refactor Signed-off-by: Suraj Banakar <[email protected]> feat: add `yaml` and `yml` in evaluator switch case feat: fix go.mod - add json-iterator package Signed-off-by: Suraj Banakar <[email protected]>
Signed-off-by: Suraj Banakar <[email protected]>
Signed-off-by: Suraj Banakar <[email protected]>
- yaml.v2 has a bug where the sub-maps are converted to `map[interface]interface{}` instead of `map[string]interface{}` Signed-off-by: Suraj Banakar <[email protected]>
- update go.mod and go.sum Signed-off-by: Suraj Banakar <[email protected]>
- didn't make a lot of sense to me Signed-off-by: Suraj Banakar <[email protected]>
Signed-off-by: Suraj Banakar <[email protected]>
- don't check the file type in when fetching file in filepathsync Signed-off-by: Suraj Banakar <[email protected]>
Signed-off-by: Suraj Banakar <[email protected]>
This reverts commit a2efb06. Signed-off-by: Suraj Banakar <[email protected]>
- not sure if this is the best way to do this Signed-off-by: Suraj Banakar <[email protected]>
Signed-off-by: Suraj Banakar <[email protected]>
Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Suraj Banakar(बानकर) | スラジ <[email protected]>
- use case X,Y instead of case X -> fallthrough -> case Y - former is cleaner than latter Signed-off-by: vadasambar <[email protected]>
Signed-off-by: vadasambar <[email protected]>
Signed-off-by: vadasambar <[email protected]>
c11e0df
to
ecd7eff
Compare
🤖 I have created a release *beep* *boop* --- ## [0.3.0](v0.2.7...v0.3.0) (2023-01-06) ### ⚠ BREAKING CHANGES * consolidated configuration change events into one event ([#241](#241)) ### Features * consolidated configuration change events into one event ([#241](#241)) ([f9684b8](f9684b8)) * support yaml evaluator ([#206](#206)) ([2dbace5](2dbace5)) ### Bug Fixes * changed eventing configuration mutex to rwmutex and added missing lock ([#220](#220)) ([5bbef9e](5bbef9e)) * omitempty targeting field in Flag structure ([#247](#247)) ([3f406b5](3f406b5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR
Related Issues
Fixes #180
Notes
I have added extra notes as comments in the code.
Follow-up Tasks
TBD
How to test
Not sure what would be a good test for this.
To try it out, you can run the example yaml config