feat: support yaml evaluator by vadasambar · Pull Request #206 · open-feature/flagd

vadasambar · 2022-11-02T18:40:36Z

This PR

adds support for YAML evaluator

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

go run main.go start -f config/samples/example_flags.yaml  -e=yaml

vadasambar · 2022-11-02T18:43:05Z

This PR is WIP. I will request review once I am done by mentioning the reviewer in a comment on this issue.

toddbaert · 2022-11-03T12:17:52Z

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?

pkg/sync/filepath_sync.go

config/samples/example_flags.yaml

pkg/sync/filepath_sync.go

beeme1mr

Could you please also update the configuration documentation?

https://github.com/open-feature/flagd/blob/main/docs/configuration.md

pkg/sync/filepath_sync.go

vadasambar · 2022-12-01T18:15:48Z

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. 🙏

beeme1mr · 2022-12-06T14:09:00Z

Hi @vadasambar, could you please fix the conflicts and resolve the issue with the test?

vadasambar · 2023-01-03T17:54:03Z

@vadasambar ./flagd start --uri file://home/todd/git/flagd/config/samples/example_flags.yaml -e=yaml works as expected, but

./flagd start --uri file://home/todd/git/flagd/config/samples/example_flags.yaml errors (I believe because in that case it anticipates JSON). I think it would improve usability to default to the evaluator matching the file extension. Do you agree?

What do you think @Kavindu-Dodan ?

Makes sense to me. 👍

vadasambar · 2023-01-03T17:54:25Z

vadasambar

Detecting flag configuration format based on file extension is a great usability improvement. However, I think this require substantial code change as well as proper documentation.

I created a follow-up issue #240, where we could have dedicated focus on this

Thank you for creating the issue 🙏

vadasambar · 2023-01-03T17:55:01Z

Approving with #206 (comment)

Thanks again for all your effort, @vadasambar

Not at all. Thank you for the feedback and review 🙏

vadasambar · 2023-01-03T17:56:14Z

I think we need review from @AlexsJones to merge this PR (not sure if we want to keep it open until #240 is done)

vadasambar · 2023-01-03T17:56:41Z

I think we need review from @AlexsJones

toddbaert · 2023-01-03T18:33:35Z

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 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]>

- 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]>

🤖 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>

vadasambar force-pushed the feat/180/add-yaml-evaluator branch from a51f64d to 19a66fb Compare November 2, 2022 18:41

This was referenced Nov 2, 2022

Oct 2022 vadafoss/daily-updates#1

Closed

Nov 2022 vadafoss/daily-updates#2

Closed

toddbaert requested review from james-milligan, skyerus and toddbaert November 3, 2022 12:14

vadasambar force-pushed the feat/180/add-yaml-evaluator branch 2 times, most recently from e832622 to cef8463 Compare November 11, 2022 19:05

AlexsJones reviewed Nov 15, 2022

View reviewed changes

pkg/sync/filepath_sync.go Outdated Show resolved Hide resolved

vadasambar force-pushed the feat/180/add-yaml-evaluator branch 3 times, most recently from 1afef7d to 07e75c5 Compare November 22, 2022 18:42

vadasambar commented Nov 22, 2022

View reviewed changes

config/samples/example_flags.yaml Show resolved Hide resolved

vadasambar changed the title ~~feat: wip support yaml evaluator~~ feat: support yaml evaluator Nov 22, 2022

vadasambar marked this pull request as ready for review November 22, 2022 18:56

vadasambar requested review from AlexsJones and removed request for james-milligan, skyerus and toddbaert November 22, 2022 18:57

beeme1mr requested review from Kavindu-Dodan, skyerus and toddbaert November 22, 2022 21:21

toddbaert reviewed Nov 22, 2022

View reviewed changes

pkg/sync/filepath_sync.go Outdated Show resolved Hide resolved

vadasambar force-pushed the feat/180/add-yaml-evaluator branch 2 times, most recently from 62ef1b0 to 3daacc1 Compare November 30, 2022 18:19

beeme1mr reviewed Dec 1, 2022

View reviewed changes

pkg/sync/filepath_sync.go Outdated Show resolved Hide resolved

beeme1mr requested a review from AlexsJones January 3, 2023 12:37

skyerus approved these changes Jan 3, 2023

View reviewed changes

vadasambar and others added 16 commits January 3, 2023 15:35

fix: go lint error

e1dcd37

Signed-off-by: Suraj Banakar <[email protected]>

fix: re-word code comment around jsoniter.Marshal

916746b

Signed-off-by: Suraj Banakar <[email protected]>

fix: use yaml.v3 instead of yaml.v2

4d2e83a

- 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]>

fix: add constants for file types

1b0a0a2

- update go.mod and go.sum Signed-off-by: Suraj Banakar <[email protected]>

fix: remove filetype abstraction

565dd27

- didn't make a lot of sense to me Signed-off-by: Suraj Banakar <[email protected]>

docs: update info about supported cli flags

15784c1

Signed-off-by: Suraj Banakar <[email protected]>

fix: tests failing

80159bd

- don't check the file type in when fetching file in filepathsync Signed-off-by: Suraj Banakar <[email protected]>

fix: return nil instead of err when fetching file via filepathsync

a9482f4

Signed-off-by: Suraj Banakar <[email protected]>

Revert "fix: tests failing"

744b608

This reverts commit a2efb06. Signed-off-by: Suraj Banakar <[email protected]>

test: add json as the default FileType to fix tests

806fbca

- not sure if this is the best way to do this Signed-off-by: Suraj Banakar <[email protected]>

fix: small spacing typo/bug

ef4ae28

Signed-off-by: Suraj Banakar <[email protected]>

docs: fix spacing ifflag -> if flag

36a715a

Co-authored-by: Michael Beemer <[email protected]> Signed-off-by: Suraj Banakar(बानकर) | スラジ <[email protected]>

fix: refactor switch case for evaluators

9e0dff0

- use case X,Y instead of case X -> fallthrough -> case Y - former is cleaner than latter Signed-off-by: vadasambar <[email protected]>

docs: add extra wording around yaml evaluation

4b079c3

Signed-off-by: vadasambar <[email protected]>

fix: make lint failing

ecd7eff

Signed-off-by: vadasambar <[email protected]>

toddbaert force-pushed the feat/180/add-yaml-evaluator branch from c11e0df to ecd7eff Compare January 3, 2023 20:35

toddbaert requested a review from james-milligan as a code owner January 3, 2023 20:35

beeme1mr merged commit 2dbace5 into open-feature:main Jan 4, 2023

github-actions bot mentioned this pull request Jan 4, 2023

chore(main): release 0.3.0 #230

Merged

austindrenski mentioned this pull request Jan 15, 2025

[BUG] inconsistent yaml support from file:// vs s3:// #1512

Closed

Conversation

vadasambar commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

Notes

Follow-up Tasks

How to test

Uh oh!

vadasambar commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toddbaert commented Nov 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beeme1mr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vadasambar commented Dec 1, 2022

Uh oh!

beeme1mr commented Dec 6, 2022

Uh oh!

vadasambar commented Jan 3, 2023

Uh oh!

vadasambar commented Jan 3, 2023

Uh oh!

vadasambar commented Jan 3, 2023

Uh oh!

vadasambar commented Jan 3, 2023

Uh oh!

vadasambar commented Jan 3, 2023

Uh oh!

toddbaert commented Jan 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vadasambar commented Nov 2, 2022 •

edited

Loading

vadasambar commented Nov 2, 2022 •

edited

Loading