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!: Support Multiple Syncs #256

Merged
merged 22 commits into from
Dec 5, 2022
Merged

feat!: Support Multiple Syncs #256

merged 22 commits into from
Dec 5, 2022

Conversation

james-milligan
Copy link
Contributor

@james-milligan james-milligan commented Nov 23, 2022

Signed-off-by: James Milligan james@omnant.co.uk

corresponding flagd changes here

This PR

  • updates the openfeature.dev/featureflagconfiguration to allow for multiple syncs to be provided to the flagd sidecar, the annotation value is now a comma separated list with each value providing both the namespace and the CRD name separated with a /, if no namespace is provided it is assumed that the crd is in the same namespace as the deployment e.g. "end-to-end, test/end-to-end-2"
  • introduces remote sync implementation

Related Issues

#251

Notes

Follow-up Tasks

How to test

  • This is a breaking change and will not work with the existing flagd version, a build from this PR will be required to test

Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022

Codecov Report

Merging #256 (47d46c3) into main (9e55a4f) will decrease coverage by 5.72%.
The diff coverage is 49.03%.

@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
- Coverage   56.18%   50.46%   -5.73%     
==========================================
  Files           3        3              
  Lines         388      432      +44     
==========================================
  Hits          218      218              
- Misses        156      199      +43     
- Partials       14       15       +1     
Impacted Files Coverage Δ
controllers/featureflagconfiguration_controller.go 0.00% <0.00%> (ø)
webhooks/pod_webhook.go 67.66% <49.51%> (-13.42%) ⬇️

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

@james-milligan james-milligan marked this pull request as ready for review November 24, 2022 15:35
@james-milligan james-milligan marked this pull request as draft November 24, 2022 15:54
@james-milligan
Copy link
Contributor Author

converting to draft to solve versioning issues

@toddbaert
Copy link
Member

toddbaert commented Nov 24, 2022

is there a reason to go with a comma delimited string over a proper yaml array? Is it just for more backward compatibility? If so, maybe there's another way to achieve it?

Maybe we could add openfeature.dev/featureflagconfigurations (note the "s") which could be an array?

I think this would be backward compatible... is this a completely dumb suggestion? @james-milligan @skyerus @AlexsJones

EDIT: as mentioned by @james-milligan , annotations can only have string values.

webhooks/pod_webhook.go Outdated Show resolved Hide resolved
@james-milligan
Copy link
Contributor Author

is there a reason to go with a comma delimited string over a proper yaml array? Is it just for more backward compatibility? If so, maybe there's another way to achieve it?

Maybe we could add openfeature.dev/featureflagconfigurations (note the "s") which could be an array?

I think this would be backward compatible... is this a completely dumb suggestion? @james-milligan @skyerus @AlexsJones

Unfortunately the annotation values must be of type string - I agree an array would be far nicer!

@toddbaert
Copy link
Member

is there a reason to go with a comma delimited string over a proper yaml array? Is it just for more backward compatibility? If so, maybe there's another way to achieve it?
Maybe we could add openfeature.dev/featureflagconfigurations (note the "s") which could be an array?
I think this would be backward compatible... is this a completely dumb suggestion? @james-milligan @skyerus @AlexsJones

Unfortunately the annotation values must be of type string - I agree an array would be far nicer!

welp, that explains that. 😅

test/e2e/e2e.yml Outdated Show resolved Hide resolved
@james-milligan
Copy link
Contributor Author

Maybe we could add openfeature.dev/featureflagconfigurations (note the "s") which could be an array?

ill update the parsing to assume that the current requests namespace is to be used if none is defined, this will keep it backwards compatible (excluding the CRD changes required to enable the remote/http sync)

Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
@james-milligan
Copy link
Contributor Author

e2e tests will continue to fail until the corresponding PR in flagd is merged, but I have tested it locally

@james-milligan james-milligan marked this pull request as ready for review November 25, 2022 12:18
Signed-off-by: James Milligan <james@omnant.co.uk>
README.md Outdated Show resolved Hide resolved
webhooks/pod_webhook.go Outdated Show resolved Hide resolved
webhooks/pod_webhook.go Outdated Show resolved Hide resolved
webhooks/pod_webhook.go Outdated Show resolved Hide resolved
webhooks/pod_webhook.go Outdated Show resolved Hide resolved
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
webhooks/pod_webhook.go Outdated Show resolved Hide resolved
james-milligan and others added 2 commits November 29, 2022 09:24
Co-authored-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
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.

Nice work young man.
Please make sure this doesn't break the demo/watchman

beeme1mr pushed a commit to open-feature/flagd that referenced this pull request Nov 30, 2022
Corresponding OFO changes
[here](open-feature/open-feature-operator#256)
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

- refactors the start command flags to remove `--sync-provider`
- multiple `--uri` flags can be passed indicating the use of different
existing `sync-provider` types, all of which will work
- uses a prefix on the uri to define the `sync-provider`; `http(s)://`
will be passed to the http sync, `file://` will be passed to the file
path sync and the Kubernetes sync uses the following pattern
`core.openfeature.dev/{namespace}/{name}`, this will also allow for the
Kubernetes sync to watch multiple `FeatureFlagConfigurations` from
different namespaces.
- adds deprecation warning when the `--sync-provider` flag is passed as
an argument

`./flagd start --uri file://etc/flagd/end-to-end.json --uri
core.openfeature.dev/test/end-to-end-2`

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

open-feature/open-feature-operator#251

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks

### How to test
<!-- if applicable, add testing instructions under this section -->

Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <75740990+james-milligan@users.noreply.github.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
Signed-off-by: James Milligan <james@omnant.co.uk>
Signed-off-by: James Milligan <james@omnant.co.uk>
@james-milligan
Copy link
Contributor Author

This PR is ready to merge following this release
open-feature/flagd#215

@james-milligan james-milligan merged commit 3ee6075 into open-feature:main Dec 5, 2022
@james-milligan james-milligan deleted the multiple-flag-config branch December 5, 2022 13:47
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.

5 participants