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: structured the featureflagconfiguration CRD #207

Merged
merged 14 commits into from
Nov 15, 2022

Conversation

skyerus
Copy link
Contributor

@skyerus skyerus commented Nov 2, 2022

This PR

Introduces v1alpha2 version of the FeatureFlagConfiguration custom resource definition.
In this version the FeatureFlagSpec field is structured (aside from variants and targeting, these fields are unstructured objects due to the structure/types being loose), whereas in v1alpha1 the FeatureFlagSpec is a string of json.

This is backwards compatible and makes use of the conversion webhook to effectively convert and store any v1alpha2 CRDs as v1alpha1.

Related Issues

Fixes #72

Notes

Follow-up Tasks

How to test

@skyerus
Copy link
Contributor Author

skyerus commented Nov 2, 2022

Converted to draft to ensure it's merged at the same time as the flagd component. Ready for review

@toddbaert
Copy link
Member

I think this is a big improvement, even if just for the syntax support in editors, but for a lot of other reasons. I will look at this and the referenced PR today.

@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2022

Codecov Report

Merging #207 (e2f73e0) into main (28c8f36) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #207   +/-   ##
=======================================
  Coverage   53.16%   53.16%           
=======================================
  Files           3        3           
  Lines         363      363           
=======================================
  Hits          193      193           
  Misses        156      156           
  Partials       14       14           
Impacted Files Coverage Δ
webhooks/featureflagconfiguration_webhook.go 86.36% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@skyerus skyerus changed the title feat!: structured the featureflagconfiguration CRD feat: structured the featureflagconfiguration CRD Nov 3, 2022
@toddbaert toddbaert self-requested a review November 3, 2022 20:49
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.

Tested locally, worked as expected with the workload from the docs: https://raw.githubusercontent.com/open-feature/playground/main/config/k8s/end-to-end.yaml

I'm going to preemptively approve this but I think @beeme1mr is correct that you might need to add $evaluators to the schema.... Probably just as a schemaless object.

Copy link
Member

@AlexsJones AlexsJones left a comment

Choose a reason for hiding this comment

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

The ambition to structure the JSON is good but trying to support two version of an API inside a single resource is chaotic. I advise you rework this to v1alphav2 it also means we will need either an upgrade path or a second reconcile loop for that API version.

@AlexsJones
Copy link
Member

I also wanted to add that, given we're in an alpha state in terms of Semver, it probably makes sense to make this a breaking change and to not try and support two versions of the API at all. I'd recommend modifying the CRD objectively and not worry about using an v1alphav2

@skyerus
Copy link
Contributor Author

skyerus commented Nov 8, 2022

Thanks for the feedback, I agree that my initial approach was unwise. I've been working on incrementing the version of the CRD in the correct manor, it's not far from completion.
Despite being in alpha I figured that it'd be painful to introduce this as a breaking change as it may create friction for people who, for example, update their version of OFO but not flagd (and vice-versa). I'd prefer to keep it as a non-breaking change for this reason.

@skyerus skyerus marked this pull request as ready for review November 8, 2022 17:15
@beeme1mr
Copy link
Member

beeme1mr commented Nov 8, 2022

Please make sure to update examples and the readme with the new CRD version.

README.md Outdated Show resolved Hide resolved
apis/core/v1beta1/featureflagconfiguration_types.go Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

go mod tidy leaves deltas for me.

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.

https://github.com/open-feature/open-feature-operator/pull/207/files#r1018167259

Tested locally... besides this, things seem to be working.

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
…r to allow backwards compatibility

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
…ion webhook to v1alpha1

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
…nfiguration CRD

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
…nd-to-end.yaml.

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
@skyerus skyerus merged commit 675724a into open-feature:main Nov 15, 2022
@skyerus skyerus deleted the issue-72_crd-structure branch November 15, 2022 09:35
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.

Improve flagD CRD Structure
5 participants