feat: add flag metadata to FeatureFlag CRD#808
feat: add flag metadata to FeatureFlag CRD#808Thuvaraki wants to merge 3 commits intoopen-feature:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds metadata support to the FeatureFlag CRD at both the spec and per-flag levels. The changes to the CRD definition and Go types are mostly correct. However, there's a significant issue with how unknown metadata fields are handled. The current implementation will lead to data loss in the operator for any metadata fields other than the explicitly defined flagSetId. Additionally, the auto-generated DeepCopy functions are performing shallow copies, which is a latent bug and a maintainability concern.
| // FlagMetadata captures optional metadata for a flag specification. | ||
| // PreserveUnknownFields keeps compatibility with flagd's metadata inheritance model. | ||
| // +kubebuilder:pruning:PreserveUnknownFields | ||
| type FlagMetadata struct { | ||
| // +optional | ||
| FlagSetID string `json:"flagSetId,omitempty"` | ||
| } |
There was a problem hiding this comment.
The FlagMetadata struct is defined to only contain FlagSetID. While +kubebuilder:pruning:PreserveUnknownFields is used on the CRD, Go's encoding/json unmarshaler will drop any fields not defined in this struct when the operator reads the resource. This contradicts the intent described in the comments to preserve additional metadata keys for flagd inheritance and will lead to data loss.
To correctly preserve unknown fields, consider using json.RawMessage for the Metadata field in FlagSpec and Flag structs. This is consistent with how Evaluators, Variants, and Targeting fields are handled in this file.
There was a problem hiding this comment.
Fixed — replaced FlagMetadata struct with json.RawMessage for both FlagSpec.Metadata and Flag.Metadata. This is now consistent with how Evaluators, Variants, and Targeting are handled.
| if in.Metadata != nil { | ||
| in, out := &in.Metadata, &out.Metadata | ||
| *out = new(FlagMetadata) | ||
| **out = **in | ||
| } |
There was a problem hiding this comment.
The generated DeepCopyInto logic for Metadata performs a shallow copy of the FlagMetadata struct. This pattern is repeated for FlagSpec.Metadata and in FlagMetadata.DeepCopyInto.
While this is currently safe because FlagMetadata only contains a string, it's a latent bug. If FlagMetadata is ever modified to include types that are not safe to shallow copy (like slices or pointers), this will lead to shared state between copied objects, violating the contract of DeepCopy.
This issue in the generated code is a symptom of the FlagMetadata type definition, which should be revised to properly handle unknown fields.
There was a problem hiding this comment.
Fixed — since Metadata is now json.RawMessage (a byte slice), the generated deepcopy correctly uses make and copy instead of a shallow struct copy."
Thuvaraki
force-pushed
the
feature/flag-metadata
branch
from
44fabaa to
d3b3256
Compare
|
Hi @justinabrahms @beeme1mr @toddbaert @oxddr @lukas-reining @odubajDT @thisthat @bacherfl Could you please review this PR when you get a chance? This PR adds metadata support to the FeatureFlag CRD Changes included:
Related PR: Happy to make any adjustments based on your feedback. Thank you! |
lukas-reining
left a comment
lukas-reining
left a comment
There was a problem hiding this comment.
Thank you @Thuvaraki! This looks good even though it only makes sense with open-feature/flagd#1905
I left two questions.
| newName: open-feature-operator-local | ||
| newTag: validate |
There was a problem hiding this comment.
Thanks for catching this - apologies.
This was a local testing change and not intended for the PR. I’ve reverted it to the original value.
go.mod
Outdated
| replace github.com/open-feature/open-feature-operator/apis => ./api | ||
|
|
There was a problem hiding this comment.
This was added temporarily for local development. I used the replace directive to point to local API types while regenerating CRDs. It was not intended to be committed. I have removed it from the PR. Both issues were due to local development setup and have now been cleaned up. The PR is updated, please let me know if anything else needs attention.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #808 +/- ##
===========================================
- Coverage 86.51% 72.33% -14.19%
===========================================
Files 19 30 +11
Lines 1587 1923 +336
===========================================
+ Hits 1373 1391 +18
- Misses 173 484 +311
- Partials 41 48 +7
... and 25 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: THuvaraki <Thuvaraki.E@cloudsolutions.com.sa>
Signed-off-by: THuvaraki <Thuvaraki.E@cloudsolutions.com.sa>
Signed-off-by: THuvaraki <Thuvaraki.E@cloudsolutions.com.sa>
Thuvaraki
force-pushed
the
feature/flag-metadata
branch
from
1c35109 to
5ed3246
Compare
2 participants
This PR
spec.flagSpec.metadata.flagSetIdspec.flagSpec.flags.<name>.metadata.flagSetIdx-kubernetes-preserve-unknown-fields: trueon both levels soflagd metadata inheritance works
Related Issues
Fixes #754
Notes
make generateandmake release-manifestsare not affected
How to test
make release-manifestsand inspectconfig/rendered/release.yamlfor both
metadatablocks underflagSpecspec.flagSpec.metadata.flagSetIdand per-flagmetadatafields and confirm it is accepted