Skip to content

feat: adds RequireFlagsEnabled decorator #1159

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Apr 24, 2025
Merged

feat: adds RequireFlagsEnabled decorator #1159

merged 10 commits into from
Apr 24, 2025

Conversation

kaushalkapasi
Copy link
Contributor

@kaushalkapasi kaushalkapasi commented Mar 14, 2025 •
edited
Loading

This PR

  • Feature: Adds a RequireFlagsEnabled decorator to allow a simple, reusable way to block access to a specific controller or endpoint based on the value of a list of one, or many, boolean flags

Notes

  • Discussions on the approach & implementation are welcome!

Follow-up Tasks

  • Update OpenFeature NestJS docs to include new RequireFlagsEnabled decorator & usage examples

How to test

npx jest --selectProject=nest

@beeme1mr beeme1mr requested a review from lukas-reining March 18, 2025 21:15
@toddbaert
Copy link
Member

I think this is a good idea, and it "feels right" to me ergonomically.

Make sure you add something to the test-app to test and document this.

@kaushalkapasi kaushalkapasi force-pushed the feat/require-flags-decorator branch 2 times, most recently from ca912f0 to a67cc35 Compare March 25, 2025 17:59
@kaushalkapasi kaushalkapasi marked this pull request as ready for review March 25, 2025 18:00
@kaushalkapasi kaushalkapasi requested a review from a team as a code owner March 25, 2025 18:00
lukas-reining
lukas-reining approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

I really like the addition!
Added some optional additions to it. Feel free to add them, otherwise we can add them in another PR :)
Thank you!

@lukas-reining
Copy link
Member

I will have a look at what is wrong with the e2e test tomorrow @kaushalkapasi :)

@toddbaert
Copy link
Member

toddbaert commented Mar 27, 2025 •
edited
Loading

I will have a look at what is wrong with the e2e test tomorrow @kaushalkapasi :)

@lukas-reining @kaushalkapasi

Seems to me like the test-harness git submodule was simply deleted. I've added it back in the latest commit. The e2e tests are working now but you have some small lint errors causing CI failures.

toddbaert
toddbaert reviewed Mar 27, 2025
View reviewed changes
toddbaert
toddbaert reviewed Mar 27, 2025
View reviewed changes
@toddbaert toddbaert self-requested a review March 27, 2025 19:16
toddbaert
toddbaert requested changes Mar 27, 2025
View reviewed changes
Copy link
Member

@toddbaert toddbaert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have a couple questions, and one request for an additional test (please let me know if you think this is not necessary) but this looks really cool. Great work!

Happy to approve from my perspective after these are resolved.

@kaushalkapasi kaushalkapasi force-pushed the feat/require-flags-decorator branch from aa03a3a to ba2c4d6 Compare March 31, 2025 15:53
@kaushalkapasi kaushalkapasi requested a review from toddbaert March 31, 2025 15:54
@lukas-reining
Copy link
Member

Sorry for taking time @kaushalkapasi, many of us have been at KubeCon.
Will have a look tomorrow or the day after :)

@kaushalkapasi kaushalkapasi force-pushed the feat/require-flags-decorator branch from ba2c4d6 to f056ae6 Compare April 7, 2025 13:49
@kaushalkapasi kaushalkapasi requested a review from a team as a code owner April 7, 2025 13:49
lukas-reining
lukas-reining reviewed Apr 7, 2025
View reviewed changes
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

It would be great if you could also add something to the README to show the feature and explain the usage, so people know it exists and how to use :)

@lukas-reining lukas-reining requested a review from a team April 7, 2025 14:18
toddbaert
toddbaert approved these changes Apr 10, 2025
View reviewed changes
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.

Implementation looks great. Like @lukas-reining says I think we could use some docs so people know how to use this cool new feature... but I'm approving.

kaushalkapasi and others added 6 commits April 21, 2025 21:53
@kaushalkapasi
feat: adds RequireFlagsEnabled decorator to allow reusable controll…
2911985 
…er & endpoint access based on boolean flag values

Signed-off-by: Kaushal Kapasi <[email protected]>
@kaushalkapasi
fix: update imports for types and cleanup js docs on the require flag…
457e272 
…s enabled decorator

Signed-off-by: Kaushal Kapasi <[email protected]>
@kaushalkapasi
feat: add tests for RequireFlagsEnabled decorator
60b1fc5 
Signed-off-by: Kaushal Kapasi <[email protected]>
@kaushalkapasi
feat: update options for RequireFlagsEnabled decorator to include con…
32a3e7f 
…text and allow for flags to change the default value for flag evaluation. move getClientForEvaluation method to utils file

Signed-off-by: Kaushal Kapasi <[email protected]>
@toddbaert @kaushalkapasi
fixup: restore sm
c99945d 
Signed-off-by: Todd Baert <[email protected]>
@kaushalkapasi
chore: remove unused import to fix lint errors
0118d87 
Signed-off-by: Kaushal Kapasi <[email protected]>
@kaushalkapasi
fix: update docs for context definition on RequireFlagsEnabledProps. …
a68c228 
…add tests with targeting rules defined for the InMemoryProvider

Signed-off-by: Kaushal Kapasi <[email protected]>
@kaushalkapasi kaushalkapasi force-pushed the feat/require-flags-decorator branch from c7f7d0e to a0028a2 Compare April 22, 2025 01:54
@kaushalkapasi kaushalkapasi force-pushed the feat/require-flags-decorator branch from a0028a2 to 2957832 Compare April 22, 2025 02:29
@beeme1mr beeme1mr added this pull request to the merge queue Apr 24, 2025
Merged via the queue into open-feature:main with commit 59b8fe9 Apr 24, 2025
10 checks passed
@openfeaturebot openfeaturebot mentioned this pull request Apr 24, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukas-reining lukas-reining lukas-reining approved these changes

@beeme1mr beeme1mr beeme1mr approved these changes

@toddbaert toddbaert toddbaert approved these changes

@luizgribeiro luizgribeiro Awaiting requested review from luizgribeiro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kaushalkapasi @toddbaert @lukas-reining @beeme1mr