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: return flag metadata as well #1476

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aasifkhan7
Copy link

@aasifkhan7 aasifkhan7 commented Dec 16, 2024

This PR

  • Adds functionality to return flag metadata as part of the response.

Related Issues

Fixes #1464

Notes

This PR ensures that flag metadata is included and handled correctly in responses.

@aasifkhan7 aasifkhan7 requested a review from a team as a code owner December 16, 2024 21:43
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 16, 2024
Copy link

netlify bot commented Dec 16, 2024

Deploy Preview for polite-licorice-3db33c ready!

Name Link
🔨 Latest commit 5cb824c
🔍 Latest deploy log https://app.netlify.com/sites/polite-licorice-3db33c/deploys/676523913f538500087b5caf
😎 Deploy Preview https://deploy-preview-1476--polite-licorice-3db33c.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@aasifkhan7 aasifkhan7 changed the title return flag metadata as well feat: return flag metadata as well Dec 17, 2024
@aasifkhan7 aasifkhan7 force-pushed the feat/flag-metadata branch 3 times, most recently from 3b6e501 to c55537f Compare December 17, 2024 05:20
Signed-off-by: Aasif Khan <aasifk106@gmail.com>
@beeme1mr
Copy link
Member

Hey @aasifkhan7, I tested this locally and wasn't able to see flag metadata returned as a response.

Here's the flag configuration I was using:

{
  "flags": {
    "myBoolFlag": {
      "state": "ENABLED",
      "variants": {
        "on": true,
        "off": false
      },
      "defaultVariant": "on"
    }
  },
  "metadata": {
    "id": "test",
    "version": "1"
  }
}

Here are the test evaluations I tried:

curl -X POST "localhost:8013/flagd.evaluation.v1.Service/ResolveBoolean" -d '{"flagKey":"myBoolFlag","context":{}}' -H "Content-Type: application/json"

and

curl -X POST 'http://localhost:8016/ofrep/v1/evaluate/flags'

Please map the id and version to flagSetId and flagSetVersion respectively in the flag metadata. That will set us up nicely once this spec PR is merged. Thanks!

Signed-off-by: Aasif Khan <aasifk106@gmail.com>
@aasifkhan7
Copy link
Author

Hi @beeme1mr,

I've updated the commit to include the setting of flagSetId and flagSetVersion in the flag metadata as well. Could you please re-review the 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.

@aasifkhan7 nice job so far!

OFREP works as expected:

curl -X POST 'http://localhost:8016/ofrep/v1/evaluate/flags' | jq

{
  "flags": [
    {
      "value": 1,
      "key": "myIntFlag",
      "reason": "STATIC",
      "variant": "one",
      "metadata": {
        "flagSetId": "test",
        "flagSetVersion": "1"
      },
    ...
    }
  ]
}
curl -X POST "localhost:8013/flagd.evaluation.v1.Service/ResolveBoolean" -d '{"flagKey":"myBoolFlag","context":{}}' -H "Content-Type: application/json" | jq


{
  "value": true,
  "reason": "STATIC",
  "variant": "on",
  "metadata": {
    "flagSetId": "test",
    "flagSetVersion": "1"
  }
}

I left some comments - I also think we need some basic test coverage for this, but nice job!

Comment on lines 335 to 340
if flagMetadata.ID != "" {
metadata[ID] = flagMetadata.ID
}
if flagMetadata.Version != "" {
metadata[VERSION] = flagMetadata.Version
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the value of these is, since we are not returning them in any responses. Maybe I'm confused.

Copy link
Author

@aasifkhan7 aasifkhan7 Dec 18, 2024

Choose a reason for hiding this comment

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

@toddbaert The idea behind this is that each flag could also have its own specific metadata. For example, a flag configuration might look like this:

{
  "flags": {
    "myBoolFlag": {
      "state": "ENABLED",
      "variants": {
        "on": true,
        "off": false
      },
      "defaultVariant": "on",
      "metadata": {
        "id": "sso/dev",
        "version": "1.0.0"
      }
    }
  },
  "metadata": {
    "id": "test",
    "version": "1"
  }
}

Here, the flag-level metadata (id, version) allows for more granular identification and versioning of individual flags, in addition to the overarching metadata. Let me know your thoughts!

core/pkg/store/flags.go Outdated Show resolved Hide resolved
Signed-off-by: Aasif Khan <aasifk106@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update flagd RPC and OFREP to support metadata
3 participants