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.

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

feat: Update in-process resolver to support flag metadata #1102 #1122

Merged
merged 19 commits into from
Jan 14, 2025
Merged

feat: Update in-process resolver to support flag metadata #1102 #1122

merged 19 commits into from
Jan 14, 2025

Conversation

chrfwow
Copy link
Contributor

@chrfwow chrfwow commented Jan 2, 2025

This PR

Adds support for flag metadata in the in-process resolver

Related Issues

Fixes #1102

@toddbaert toddbaert closed this Jan 6, 2025
@toddbaert toddbaert reopened this Jan 6, 2025
@toddbaert toddbaert marked this pull request as ready for review January 6, 2025 19:37
@toddbaert toddbaert requested a review from a team as a code owner January 6, 2025 19:37
@toddbaert toddbaert marked this pull request as draft January 6, 2025 19:38
@toddbaert toddbaert force-pushed the feat/Update-in-process-resolver-to-support-flag-metadata branch 3 times, most recently from 1fa47fc to 1285149 Compare January 6, 2025 20:52
@toddbaert toddbaert force-pushed the feat/Update-in-process-resolver-to-support-flag-metadata branch from 1285149 to c08e7bc Compare January 6, 2025 20:52
@toddbaert toddbaert mentioned this pull request Jan 6, 2025
Closed
@toddbaert
Copy link
Member

toddbaert commented Jan 6, 2025 •
edited
Loading

@chrfwow this is definitely a step in the right direction - I think there's probably some null check missing related to the metadata causing the e2e failures: Run 1: Cannot invoke "dev.openfeature.sdk.Structure.asMap()" because "structure" is null

Working with @beeme1mr and a few others in in slack, we've decided another thing we'd want here is a very basic mechanism for reducing duplication: the ability to add metadata for the entire flag set at the "top level" which all flags inherit:

{
  "metadata": {
    "foo": "bar"
  },
  "flags": {
    "flag1": {
      // ...
      "metadata": {
        // "foo":  "bar" is automatically included, but can be overridden if defined in `flag1`
      }
    }
  }
}

The same thing is implemented here: open-feature/js-sdk-contrib#1120

See: https://github.com/open-feature/flagd-schemas/blob/37baa2cdea48a5ac614ba3e718b7d02ad4120611/json/flags.json#L53

cc @beeme1mr

@chrfwow
Merge remote-tracking branch 'origin/feat/Update-in-process-resolver-…
4c6393a 
…to-support-flag-metadata' into feat/Update-in-process-resolver-to-support-flag-metadata

# Conflicts:
#	providers/flagd/schemas
@chrfwow chrfwow requested a review from aepfli January 9, 2025 09:49
@toddbaert toddbaert self-requested a review January 9, 2025 20:48
@toddbaert toddbaert self-requested a review January 9, 2025 20:52
toddbaert
toddbaert requested changes Jan 9, 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.

I found one small issue here

@chrfwow chrfwow marked this pull request as draft January 10, 2025 07:11
@chrfwow
fixup! feat: Update in-process resolver to support flag metadata open…
65a508c 
…-feature#1102

Signed-off-by: christian.lutnik <[email protected]>
@chrfwow
Merge remote-tracking branch 'origin/feat/Update-in-process-resolver-…
9b49abc 
…to-support-flag-metadata' into feat/Update-in-process-resolver-to-support-flag-metadata

# Conflicts:
#	providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/InProcessResolver.java
@chrfwow
fixup! feat: Update in-process resolver to support flag metadata open…
a8a2cd6 
…-feature#1102

Signed-off-by: christian.lutnik <[email protected]>
@chrfwow chrfwow requested a review from toddbaert January 10, 2025 10:33
@chrfwow chrfwow marked this pull request as ready for review January 13, 2025 07:01
@toddbaert toddbaert self-requested a review January 13, 2025 17:31
@toddbaert toddbaert merged commit a330bd6 into open-feature:main Jan 14, 2025
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 14, 2025
Open
@chrfwow chrfwow deleted the feat/Update-in-process-resolver-to-support-flag-metadata branch January 15, 2025 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddbaert toddbaert toddbaert approved these changes

@beeme1mr beeme1mr beeme1mr approved these changes

@ajhelsby ajhelsby Awaiting requested review from ajhelsby

@Kavindu-Dodan Kavindu-Dodan Awaiting requested review from Kavindu-Dodan

@matthewelwell matthewelwell Awaiting requested review from matthewelwell

@thisthat thisthat Awaiting requested review from thisthat

@aepfli aepfli Awaiting requested review from aepfli

Assignees

@beeme1mr beeme1mr

@thisthat thisthat

@Kavindu-Dodan Kavindu-Dodan

@matthewelwell matthewelwell

@ajhelsby ajhelsby

@toddbaert toddbaert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[flagd] Update in-process resolver to support flag metadata
8 participants
@chrfwow @toddbaert @beeme1mr @aepfli @thisthat @Kavindu-Dodan @matthewelwell @ajhelsby