Skip to content
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

[FEATURE] Support addition of arbitrary properties in evaluation context #1435

Open
4 tasks
toddbaert opened this issue Oct 29, 2024 · 5 comments · May be fixed by #1448
Open
4 tasks

[FEATURE] Support addition of arbitrary properties in evaluation context #1435

toddbaert opened this issue Oct 29, 2024 · 5 comments · May be fixed by #1448
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@toddbaert
Copy link
Member

toddbaert commented Oct 29, 2024

Requirements

Currently, flagd injects a couple standard properties into the evaluation context ($flagd.flagKey, $flagd.timestamp).

In addition, we want to support the addition of ANY arbitrary string properties specified on the command line.
The command line should look something like this:

flagd start -X name=jane -X age=31 ... or flagd start --context-value name=jane --context-value age=31 ...

The above example will add the key/value pairs "name": "jane" and "age": "31" to the evaluation context for every evaluation.

Note that values are all strings - if desired, JSONLogic can be used in rules to convert them to other types.
Also note that there's no prefix associated with these additional fields.

In addition to adding these values to the evaluation context for all evaluations done in the flagd daemon itself, flagd should return these values in the sync-metadata call, so that in-process providers can add these values to their evaluations as well.

  • add cmd line flag
  • enrich all evaluations with data from cmd line
  • return data from cmd line in sync-metadata
  • associated tests and README doc
@toddbaert toddbaert added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Oct 29, 2024
@alemrtv
Copy link

alemrtv commented Nov 1, 2024

hi @toddbaert, I'd like to try working on this one, could you assign it to me please?

@toddbaert
Copy link
Member Author

Yes, absolutely! Assigned.

@alemrtv
Copy link

alemrtv commented Nov 2, 2024

@toddbaert

  1. the code is mostly ready, but how would you recommend to test these changes? Currently, the tests already use the context during the flag evaluation in flagd/core/pkg/evaluator/json_test.go. I would need to test it from a much higher level, though, to make sure that the values added by the new flag are passed to Resolve...Value functions. The higher level tests, however, mock the lower level layers: example. Do I need to add some kind of an end-2-end test?

  2. Would you like me to describe the changes in the README.md in the root directory of the project or somewhere else?

@toddbaert
Copy link
Member Author

  1. I would add a very basic unit test that asserts the existence of some arbitrary new properties in the context. It might not cover things "end to end" but I suspect your new code has some method with new parameters somewhere now? I would just make sure that if those are set to include some additional props, those props are present; also make sure the sync-metadata RPC is returning them. We have a very robust e2e gherkin test suite: https://github.com/open-feature/flagd-testbed/tree/main/gherkin where we should add this eventually, but that will have to be a separate task.

  2. Good question... after thinking more, I think it'd be best if you added a small section here. Also note the command line args are self-documenting and get injected into the docsite in that section if you do make generate-docs. You can also run the web docs with make run-web-docs. I don't expect you to necessarily understand all the background here, so I'll probably add a bit to your docs; but feel free to take a pass at it! 🙏

@alemrtv alemrtv linked a pull request Nov 10, 2024 that will close this issue
@alemrtv
Copy link

alemrtv commented Nov 10, 2024

@toddbaert the PR is ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants