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

feat: standalone targeting, fractional shorthand #168

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jul 4, 2024

This PR does 2 things:

  • Allows for fractional targeting shorthand where no weight (assumes weight = 1)
  • Allows the targeting.json schema to be used alone, not just as part of the flag definition schema

Non functionally, it restructures our testing so that tests pertaining to targeting.json exist in the test/targeting/ dir, while tests pertaining to the flags.json exist in the test/flags dir.

Fixes: #165

@toddbaert toddbaert requested a review from a team as a code owner July 4, 2024 18:03
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the fix/fract-shorthand-and-targeting-alone branch from fdb4ec1 to 389625e Compare July 4, 2024 18:19
Comment on lines 7 to 9
properties:
targeting:
$ref: "#/definitions/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.

This allows for the use of targeting by itself.

@@ -0,0 +1,13 @@
{
Copy link
Member Author

@toddbaert toddbaert Jul 4, 2024

Choose a reason for hiding this comment

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

All these tests used to be embedded in flag json objects, which was not necessary for testing the targeting. This PR converts all tests which just test targeting validation to purely targeting tests, since we can now validate targeting alone.

@@ -462,7 +467,7 @@
"$comment": "if we remove the \"sum to 100\" restriction, update the descriptions below!",
"description": "Distribution for all possible variants, with their associated weighting out of 100.",
"type": "array",
"minItems": 2,
"minItems": 1,
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows for the fractional shorthand support (no wieght == weight 1)

Comment on lines 19 to 22
"fractional": [
[ "heads" ],
[ "tails", 1 ]
]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only net-new test.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert requested a review from DBlanchard88 July 4, 2024 18:29
@@ -4,6 +4,11 @@
"title": "flagd Targeting",
"description": "Defines targeting logic for flagd; a extension of JSONLogic, including purpose-built feature-flagging operations.",
"type": "object",
"properties": {
Copy link
Member

Choose a reason for hiding this comment

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

i do not think this is actually what we want. this validates an object with a property targeting like

{
  "targeting": {
    "ifddd": [
      {
        "starts_with": [
          {
            "var": "id"
          },
          "abc"
        ]
      },
      "prefix",
      "else"
    ]
  }
}

see https://www.jsonschemavalidator.net/s/yRS6TJ9g -

but what we actually want is a validation without the targeting property like in proposed in #165 (comment) something like

{
  "ifddd": [
    {
      "starts_with": [
        {
          "var": "id"
        },
        "abc"
      ]
    },
    "prefix",
    "else"
  ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely correct, thanks a lot.

I've made this change.

Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

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

so far the code and test changes do look good, but i do think we should change the schema changes, to not need the property

Comment on lines 7 to 11
"properties": {
"targeting": {
"$ref": "#/definitions/targeting"
}
},
Copy link
Member

@aepfli aepfli Jul 5, 2024

Choose a reason for hiding this comment

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

Suggested change
"properties": {
"targeting": {
"$ref": "#/definitions/targeting"
}
},
"anyOf": [
{
"$ref": "#/definitions/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.

Oh you're totally right. I've corrected this and updated all the tests!

@toddbaert toddbaert force-pushed the fix/fract-shorthand-and-targeting-alone branch 2 times, most recently from 13c185b to 2e1d608 Compare July 5, 2024 18:21
@toddbaert toddbaert requested a review from aepfli July 5, 2024 18:23
@toddbaert toddbaert force-pushed the fix/fract-shorthand-and-targeting-alone branch from 2e1d608 to 41e3d71 Compare July 5, 2024 18:25
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the fix/fract-shorthand-and-targeting-alone branch from 41e3d71 to 15a04ec Compare July 5, 2024 18:30
@toddbaert toddbaert merged commit 6211e3a into main Jul 8, 2024
6 checks passed
toddbaert pushed a commit that referenced this pull request Jul 8, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[0.2.8](json/json-schema-v0.2.7...json/json-schema-v0.2.8)
(2024-07-08)


### ✨ New Features

* standalone targeting, fractional shorthand
([#168](#168))
([6211e3a](6211e3a))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@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.

Targeting sub-schema doesn't work alone
3 participants