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 validation #1146

Merged
merged 12 commits into from
Jan 16, 2024

Conversation

olunusib
Copy link
Contributor

This PR

  • adds targeting validation
  • updates sample json files
  • updates schemas modules to most recent release

Related Issues

Closes #1140

Notes

Follow-up Tasks

How to test

Copy link

netlify bot commented Jan 15, 2024

Deploy Preview for polite-licorice-3db33c canceled.

Name Link
🔨 Latest commit b4ac675
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/65a69bc2b8809e0008a678b8

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (2adfa57) 73.53% compared to head (b4ac675) 73.38%.

Files Patch % Lines
core/pkg/evaluator/json.go 66.66% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
- Coverage   73.53%   73.38%   -0.15%     
==========================================
  Files          32       32              
  Lines        3113     3130      +17     
==========================================
+ Hits         2289     2297       +8     
- Misses        717      723       +6     
- Partials      107      110       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
@toddbaert
Copy link
Member

Hey @olunusib , nice job so far!

Please note I have just released a new schema, which fixed some bugs (I don't think it will conflict with any of your changes). Could you update the submodule again to this version?

@toddbaert
Copy link
Member

Hey @olunusib , nice job so far!

Please note I have just released a new schema, which fixed some bugs (I don't think it will conflict with any of your changes). Could you update the submodule again to this version?

Oh wow you already did! Thanks!

@toddbaert toddbaert mentioned this pull request Jan 15, 2024
…mpilation and validation

Signed-off-by: Best Olunusi <olunusibest@gmail.com>
@olunusib olunusib marked this pull request as ready for review January 15, 2024 21:16
@olunusib olunusib requested a review from a team as a code owner January 15, 2024 21:16
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
@olunusib
Copy link
Contributor Author

This PR contains a lot of logging changes and I noticed there aren't existing tests for logger functionality. Setting up mocks for the logger is quite involved, especially since an ideal solution would involve a project-wide approach or a method defined in a common location accessible across the project, not just for the file I'm editing. The code coverage bot is highlighting the untested lines though so given this, are we okay with proceeding without logger-specific tests for now, or are there any recommendations for a straightforward, scalable approach to mocking the logs?

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented Jan 15, 2024

This PR contains a lot of logging changes and I noticed there aren't existing tests for logger functionality. Setting up mocks for the logger is quite involved, especially since an ideal solution would involve a project-wide approach or a method defined in a common location accessible across the project, not just for the file I'm editing. The code coverage bot is highlighting the untested lines though so given this, are we okay with proceeding without logger-specific tests for now, or are there any recommendations for a straightforward, scalable approach to mocking the logs?

Hi, thank you for the contribution. Yes, code coverage complain as we are not covering the condition, where we handle error with a log.

A good way to address this is to add few invalid targeting rules to existing test suites 1 and expect them to parse. This will make the condition to evaluate (cause parsing error, log but continue), making the validation green. Also, this makes sure that our logic is correct - log and continue on targeting parsing errors.

Footnotes

  1. https://github.com/open-feature/flagd/blob/main/core/pkg/evaluator/json_test.go

…ation

Signed-off-by: Best Olunusi <olunusibest@gmail.com>
@olunusib
Copy link
Contributor Author

Thanks for the suggestion. I have added some configs with invalid targeting rules. Code coverage seems happy now.

core/go.mod Show resolved Hide resolved
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Great work! Only minor suggestions for adding some in-line comments around the less-obvious JSON schema parsing, and a slight improvement to one of the warning logs.

olunusib and others added 2 commits January 16, 2024 08:47
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
Signed-off-by: Best Olunusi <olunusibest@gmail.com>
@toddbaert
Copy link
Member

@olunusib don't worry about the coverage now. There's some kind of odd jitter in it that I dont' understand (might be because we're stuck on an old version for now).

Signed-off-by: Best Olunusi <olunusibest@gmail.com>
core/go.mod Show resolved Hide resolved
@toddbaert toddbaert merged commit b727dd0 into open-feature:main Jan 16, 2024
12 of 14 checks passed
@github-actions github-actions bot mentioned this pull request Jan 16, 2024
toddbaert added a commit 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>
toddbaert pushed a commit that referenced this pull request Feb 6, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.8.2</summary>

##
[0.8.2](flagd/v0.8.1...flagd/v0.8.2)
(2024-02-05)


### 🐛 Bug Fixes

* **deps:** update module github.com/open-feature/flagd/core to v0.7.4
([#1119](#1119))
([e998e41](e998e41))
* use correct link in sources flag helper text in start cmd
([#1126](#1126))
([b9d30e0](b9d30e0))
</details>

<details><summary>flagd-proxy: 0.4.2</summary>

##
[0.4.2](flagd-proxy/v0.4.1...flagd-proxy/v0.4.2)
(2024-02-05)


### 🐛 Bug Fixes

* add signal handling to SyncFlags grpc
([#1176](#1176))
([5c8ed7c](5c8ed7c))
* **deps:** update module github.com/open-feature/flagd/core to v0.7.4
([#1119](#1119))
([e998e41](e998e41))
</details>

<details><summary>core: 0.7.5</summary>

##
[0.7.5](core/v0.7.4...core/v0.7.5)
(2024-02-05)


### 🐛 Bug Fixes

* add signal handling to SyncFlags grpc
([#1176](#1176))
([5c8ed7c](5c8ed7c))
* **deps:** update kubernetes packages to v0.29.1
([#1156](#1156))
([899e6b5](899e6b5))
* **deps:** update module
buf.build/gen/go/open-feature/flagd/connectrpc/go to
v1.14.0-20231031123731-ac2ec0f39838.1
([#1170](#1170))
([8b3c8d6](8b3c8d6))
* **deps:** update module golang.org/x/crypto to v0.18.0
([#1138](#1138))
([53569d9](53569d9))
* **deps:** update module golang.org/x/net to v0.20.0
([#1139](#1139))
([fdb1d0c](fdb1d0c))
* **deps:** update module google.golang.org/grpc to v1.61.0
([#1164](#1164))
([11ccecd](11ccecd))
* **deps:** update opentelemetry-go monorepo
([#1155](#1155))
([436fefe](436fefe))


### ✨ New Features

* add targeting validation
([#1146](#1146))
([b727dd0](b727dd0))
* **core:** support any auth scheme in HTTP-sync auth header
([#1152](#1152))
([df65966](df65966))


### 🧹 Chore

* update test dependencies, fix for otel api change and update renovate
configuration
([#1188](#1188))
([3270346](3270346))


### 📚 Documentation

* update schemas
([#1158](#1158))
([396c618](396c618))
</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>
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] Add targeting validation and log warning if targeting is invalid
4 participants