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: add targeting schema, improve def. schema #120

Merged
merged 14 commits into from
Jan 8, 2024

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jan 3, 2024

This PR:

  • adds JSON-schema validation for targeting rules
    • basic JSONLogic rules validated
    • custom rules (sem_ver, fractional, starts/ends_with) are fully validated
    • descriptions are added for all operations and operands
    • $evalutators validated
    • maintained in separate schema
  • improves flag-definition schema
    • descriptions added/improved

I've also added a bunch of positive and negative tests.

For a preview, add "$schema": "https://deploy-preview-1115--polite-licorice-3db33c.netlify.app/schema/v0/flagd-definitions.json" as a sibling to "flags" in your flagd config.

Closes: #115

⚠️ KEEP IN MIND WHEN REVIEWING that that .yaml files are the source of truth; the CI checks that the json is consistent with them.

validation

@toddbaert toddbaert requested a review from a team as a code owner January 3, 2024 19:34
@toddbaert toddbaert force-pushed the feat/add-targeting-schema branch from 57925a0 to 98d7c3f Compare January 3, 2024 19:34
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert force-pushed the feat/add-targeting-schema branch from 6e420b9 to 2c6e808 Compare January 3, 2024 21:47
@toddbaert toddbaert force-pushed the feat/add-targeting-schema branch from 4cb07fe to 005112a Compare January 3, 2024 22:11
json/targeting.json Outdated Show resolved Hide resolved
@beeme1mr
Copy link
Member

beeme1mr commented Jan 4, 2024

VSCode supports a few additional properties like markdownDescription that we may want to consider leveraging.

https://code.visualstudio.com/docs/languages/json#_use-rich-formatting-in-hovers

json/targeting.json Outdated Show resolved Hide resolved
toddbaert and others added 2 commits January 4, 2024 16:25
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member Author

VSCode supports a few additional properties like markdownDescription that we may want to consider leveraging.

https://code.visualstudio.com/docs/languages/json#_use-rich-formatting-in-hovers

Seems like "defaultSnippets" also is an interesting non-standard addition it supports.

@toddbaert
Copy link
Member Author

toddbaert commented Jan 5, 2024

Marking this draft for a bit because there's a few additions I want to make.

@toddbaert toddbaert marked this pull request as draft January 5, 2024 14:17
Signed-off-by: Todd Baert <[email protected]>
@beeme1mr
Copy link
Member

beeme1mr commented Jan 5, 2024

Seems like "defaultSnippets" also is an interesting non-standard addition it supports.

Wow, that's an interesting feature!

@toddbaert toddbaert force-pushed the feat/add-targeting-schema branch from df6c094 to 69e4f99 Compare January 5, 2024 18:39
Signed-off-by: Todd Baert <[email protected]>
@toddbaert toddbaert marked this pull request as ready for review January 5, 2024 18:40
@toddbaert
Copy link
Member Author

@beeme1mr @Kavindu-Dodan I've added a bunch of tests. There was already some really nice testing implemented that simply iterates over a directory of definitions expected to fail and definitions expected to pass, so I just added a bunch of each!

@beeme1mr I've also changed the defaults for boolean, and used anyOf as you suggested (see here for more info). I didn't add any of the vscode-specific features. I don't think it's a bad idea but I'd prefer to do it in a different PR since it's non-breaking and this is already big.

@@ -4,3 +4,6 @@ import _ "embed"

//go:embed flagd-definitions.json
var FlagdDefinitions string

//go:embed targeting.json
var Targeting string
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we export a const for the Targeting schema as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. We should follow up and add validations in flagd for flag parsing 1

Footnotes

  1. https://github.com/open-feature/flagd/blob/main/core/pkg/evaluator/json.go#L404-L406

Comment on lines 20 to 27
schemaLoader = gojsonschema.NewSchemaLoader()
schemaLoader.AddSchemas(gojsonschema.NewStringLoader(flagd_definitions.Targeting))
var err error
s, err = schemaLoader.Compile(gojsonschema.NewStringLoader(flagd_definitions.FlagdDefinitions))
if err != nil {
s := fmt.Errorf("err: %v", err)
log.Fatal(s)
}
Copy link
Member Author

@toddbaert toddbaert Jan 5, 2024

Choose a reason for hiding this comment

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

@Kavindu-Dodan you asked about this... this is how we can directly import schemas in Go (notice I import the referenced ($ref) schema first, and then the root schema). This means no actual http requests will be made, the schemas are cached and referenced by their $id. In most prod systems, this is how it's done - few servers or libraries will actually make HTTP requests for schemas, that's typically more of a development feature (like for IDEs).

See this link:

Even though schemas are identified by URIs, those identifiers are not necessarily network-addressable. They are just identifiers. Generally, implementations don't make HTTP requests (https://) or read from the file system (file://) to fetch schemas. Instead, they provide a way to load schemas into an internal schema database. When a schema is referenced by it's URI identifier, the schema is retrieved from the internal schema database.

cc @beeme1mr

Copy link
Member Author

Choose a reason for hiding this comment

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

Note I improved some var-names here since I made this comment.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't even consider this but it's definitely good to know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thank you for the link and the comment. I thought this to be a valid URL hence my doubt :)

json/targeting.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

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

Awesome work 👏

Added few suggestions but I am approving this anyway.

toddbaert and others added 2 commits January 8, 2024 13:08
Co-authored-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
json/targeting.yaml Outdated Show resolved Hide resolved
@toddbaert toddbaert merged commit 6041fc7 into main Jan 8, 2024
8 checks passed
@toddbaert toddbaert deleted the feat/add-targeting-schema branch January 8, 2024 19:20
@github-actions github-actions bot mentioned this pull request Jan 8, 2024
toddbaert pushed a commit that referenced this pull request Jan 8, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[0.1.2](json/json-schema-v0.1.1...json/json-schema-v0.1.2)
(2024-01-08)


### ✨ New Features

* add targeting schema, improve def. schema
([#120](#120))
([6041fc7](6041fc7))
</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>
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.

[FEATURE] Validation and editor support via rigorous JSON schema
3 participants