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

refactor: refactor sync providers #291

Conversation

Kavindu-Dodan
Copy link
Contributor

This PR

Revisits sync providers and refactor them.

  • unify event handling
  • packaging improvements
  • removal of unused codes (cleanup)
  • clean contracts between Runtime and Sync implementations

With this change, we get a clear isolation between runtime and sync providers (ex:- file, k8s, ....). For example, the runtime is not aware of any sync implementation details such as events. It simply coordinates sync impls, store (evaluator) and notifications. This should bring more extensibility when adding future extensions.

Below is the overview of the internals and interactions,

image

@james-milligan
Copy link
Contributor

This looks like a great improvement to the current sync / eval implementation.

Ill test this properly once it is ready for review

pkg/sync/http/http_sync.go Outdated Show resolved Hide resolved
pkg/sync/http/http_sync.go Outdated Show resolved Hide resolved
pkg/sync/http/http_sync.go Outdated Show resolved Hide resolved
pkg/runtime/runtime.go Show resolved Hide resolved
pkg/sync/http/http_sync_test.go Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

go mod tidy leaves some deltas for me.

@toddbaert toddbaert self-requested a review January 20, 2023 16:07
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.

This looks really good to me. We've added to flagd a number of times, and whenever that happens it usually creates need for a refactor to clean things up. I find the new interfaces much clearer! Thanks!

I also tested this locally a bit and found no issues.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan Kavindu-Dodan force-pushed the refactor/refactor-sync-providers branch from fb85b8d to 764255f Compare January 23, 2023 15:50
@james-milligan james-milligan merged commit 46c5bf8 into open-feature:main Jan 23, 2023
@Kavindu-Dodan Kavindu-Dodan deleted the refactor/refactor-sync-providers branch January 23, 2023 16:07
toddbaert pushed a commit that referenced this pull request Jan 30, 2023
## This PR

Adds a component diagram for flag configuration providers and their
interactions with the runtime and other systems.

Related PR #291

---------

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
beeme1mr pushed a commit that referenced this pull request Feb 7, 2023
@Kavindu-Dodan has contributed multiple significant changes and
proposals to flagd:

- multiple refactors: #291,
#307
- ci/security improvements:
#338,
#337
- architectural proposals (some of which got some attention from outside
parties!): open-feature/ofep#45,
open-feature/flagd-schemas#78,
#249 (comment)
- load testing: #225
- documentation improvements

For these reasons, I believe he should be made a CODEOWNER in this
repository.

NOTE: before this is merged, @Kavindu-Dodan should be added with at
least `maintainer` permissions to the repo.

Signed-off-by: Todd Baert <toddbaert@gmail.com>
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.

4 participants