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

fix: add "/" op and fix special cases in schema #122

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

toddbaert
Copy link
Member

After @thisthat noticed I was missing "/", I added a test that with a massive targeting rule that doesn't make sense in terms of feature flagging, but uses every single default JSON logic op in their various forms. In doing that I found a missed a few special cases:

missing special cases:

"+" can be used with a single param to cast to number
"-" can be used with a single param to multiply by -1
"substr" can be used with 3 args to to return a "middle" substring

bugs:

"reduce" needs 3 args
"max/min" take an array directly and no mapper function, so they can take any amount of args
"<" and "<=" require at least 2 args

@toddbaert toddbaert requested a review from a team as a code owner January 12, 2024 17:09
@toddbaert toddbaert force-pushed the fix/json-special-cases-and-divide branch from 5dd9633 to 1ce17ae Compare January 12, 2024 17:23
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert changed the title fix: add divide and json special cases fix: add divide and json fix special cases Jan 12, 2024
@@ -1,4 +1,5 @@
{
"$schema": "../../flagd-definitions.json",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every file in this folder just had the schema added, and then JSON formatted.

},
"defaultVariant": "on",
"targeting": {
"*" : [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new test that tests EVERY single example from the JSONLogic site.

@toddbaert toddbaert changed the title fix: add divide and json fix special cases fix: add "/" op and fix special cases in schema Jan 12, 2024
json/targeting.yaml Show resolved Hide resolved
@toddbaert toddbaert merged commit 4a0be4f into main Jan 15, 2024
8 checks passed
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
toddbaert pushed a commit that referenced this pull request Jan 15, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>json/json-schema: 0.1.3</summary>

##
[0.1.3](json/json-schema-v0.1.2...json/json-schema-v0.1.3)
(2024-01-15)


### 🐛 Bug Fixes

* add "/" op and fix special cases in schema
([#122](#122))
([4a0be4f](4a0be4f))
</details>

---
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>
@beeme1mr beeme1mr deleted the fix/json-special-cases-and-divide branch January 16, 2024 15:45
toddbaert added a commit to open-feature/flagd that referenced this pull request Jan 17, 2024
Adds JSON schema at
`https://flagd.dev/schema/v0/flagd-definitions.json`, which can be used
to add validation to flagd config files.

~Note, to accomplish this cleanly, I have cloned the schemas module
twice (at different versions) one is used for json releases (and tagged
to something like `json/json-schema-v0.1.2`) while the other is for the
protobuf generation and tagged to a release of that. I don't love this,
but I think it's needed.~

I've decided against this. I don't think we'll really have conflicts
here.

See schema here:
https://deploy-preview-1115--polite-licorice-3db33c.netlify.app/schema/v0/flagd-definitions.json

Once this is available, I intend to update a bunch of docs and examples
to leverage this schema, and and an blurb in the docs about it.

~Blocked by: open-feature/flagd-schemas#122

~I think I'll wait for
[this](#1146) to be done
before adding this, to avoid conflicts.~

---------

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants