-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
fdb4ec1
to
389625e
Compare
json/targeting.yaml
Outdated
properties: | ||
targeting: | ||
$ref: "#/definitions/targeting" |
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.
This allows for the use of targeting by itself.
@@ -0,0 +1,13 @@ | |||
{ |
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.
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, |
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.
This allows for the fractional shorthand support (no wieght == weight 1)
"fractional": [ | ||
[ "heads" ], | ||
[ "tails", 1 ] | ||
] |
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.
This is the only net-new test.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
json/targeting.json
Outdated
@@ -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": { |
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.
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"
]
}
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.
Absolutely correct, thanks a lot.
I've made this change.
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.
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
json/targeting.json
Outdated
"properties": { | ||
"targeting": { | ||
"$ref": "#/definitions/targeting" | ||
} | ||
}, |
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.
"properties": { | |
"targeting": { | |
"$ref": "#/definitions/targeting" | |
} | |
}, | |
"anyOf": [ | |
{ | |
"$ref": "#/definitions/targeting" | |
} | |
], |
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.
Oh you're totally right. I've corrected this and updated all the tests!
13c185b
to
2e1d608
Compare
2e1d608
to
41e3d71
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
41e3d71
to
15a04ec
Compare
🤖 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>
This PR does 2 things:
Non functionally, it restructures our testing so that tests pertaining to
targeting.json
exist in thetest/targeting/
dir, while tests pertaining to theflags.json
exist in thetest/flags
dir.Fixes: #165