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: introduce per-sync configurations #448

Merged
merged 36 commits into from
Mar 9, 2023
Merged

feat: introduce per-sync configurations #448

merged 36 commits into from
Mar 9, 2023

Conversation

james-milligan
Copy link
Contributor

@james-milligan james-milligan commented Feb 28, 2023

This PR

  • introduces the SyncProviderConfig object and associated start flags to pass configuration to specific sync providers

Related Issues

#405

Notes

Follow-up Tasks

How to test

Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #448 (2dd6912) into main (6a039ce) will decrease coverage by 6.40%.
The diff coverage is 46.75%.

@@            Coverage Diff             @@
##             main     open-feature/flagd#448      +/-   ##
==========================================
- Coverage   68.47%   62.08%   -6.40%     
==========================================
  Files          13       15       +2     
  Lines        1656     1891     +235     
==========================================
+ Hits         1134     1174      +40     
- Misses        461      654     +193     
- Partials       61       63       +2     
Impacted Files Coverage Δ
pkg/runtime/runtime.go 0.00% <ø> (ø)
pkg/sync/file/filepath_sync.go 54.54% <ø> (ø)
pkg/sync/grpc/grpc_sync.go 62.04% <ø> (ø)
pkg/sync/http/http_sync.go 43.29% <0.00%> (ø)
pkg/sync/kubernetes/kubernetes_sync.go 83.55% <ø> (-0.08%) ⬇️
pkg/runtime/from_config.go 25.30% <47.36%> (ø)

📣 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 March 1, 2023 12:06
cmd/start.go Outdated Show resolved Hide resolved
pkg/runtime/from_config.go Show resolved Hide resolved
pkg/runtime/from_config.go Outdated Show resolved Hide resolved
james-milligan and others added 3 commits March 1, 2023 13:51
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

I wasn't able to get the examples working. I built flagd using your branch and confirmed the config example flags were in the expected location.

./flagd start --sync-providers=\[{\"uri\":\"config/samples/example_flags.json\"\,\"provider\":\"file\"}\]

                 ______   __       ________   _______    ______      
                /_____/\ /_/\     /_______/\ /______/\  /_____/\     
                \::::_\/_\:\ \    \::: _  \ \\::::__\/__\:::_ \ \    
                 \:\/___/\\:\ \    \::(_)  \ \\:\ /____/\\:\ \ \ \   
                  \:::._\/ \:\ \____\:: __  \ \\:\\_  _\/ \:\ \ \ \  
                   \:\ \    \:\/___/\\:.\ \  \ \\:\_\ \ \  \:\/.:| | 
                    \_\/     \_____\/ \__\/\__\/ \_____\/   \____/_/ 

2023-03-01T14:01:42.843-0500    info    cmd/start.go:104        flagd version: dev (5ecdb19), built at: 2023-03-01T14:00:02Z    {"component": "start"}
2023-03-01T14:01:42.843-0500    fatal   cmd/start.go:144        no sync implementation set      {"component": "start"}
github.com/open-feature/flagd/cmd.glob..func1
        /home/beemer/code/openfeature/flagd/cmd/start.go:144
github.com/spf13/cobra.(*Command).execute
        /home/beemer/go/pkg/mod/github.com/spf13/[email protected]/command.go:920
github.com/spf13/cobra.(*Command).ExecuteC
        /home/beemer/go/pkg/mod/github.com/spf13/[email protected]/command.go:1044
github.com/spf13/cobra.(*Command).Execute
        /home/beemer/go/pkg/mod/github.com/spf13/[email protected]/command.go:968
github.com/open-feature/flagd/cmd.Execute
        /home/beemer/code/openfeature/flagd/cmd/root.go:37
main.main
        /home/beemer/code/openfeature/flagd/main.go:30
runtime.main
        /home/beemer/sdk/go1.19.6/src/runtime/proc.go:250

docs/configuration/flagd_start.md Outdated Show resolved Hide resolved
docs/configuration/flagd_start.md Outdated Show resolved Hide resolved
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented Mar 7, 2023

Overal looks good 👍

Please check linting failures

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

It's probably unrelated to this change but I'm seeing two WRITE events in the logs every time a flag configuration changes using the file source. Could you please confirm that this change isn't the root cause. If it's not, please create a follow up issue to investigate.

docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
docs/configuration/configuration.md Outdated Show resolved Hide resolved
cmd/start.go Show resolved Hide resolved
@james-milligan
Copy link
Contributor Author

It's probably unrelated to this change but I'm seeing two WRITE events in the logs every time a flag configuration changes using the file source. Could you please confirm that this change isn't the root cause. If it's not, please create a follow up issue to investigate.

Please can you provide more info - im not able to replicate this behaviour:

2023-03-08T09:27:41.069Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.740Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json CHMOD {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.741Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json WRITE {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.741Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json CHMOD {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json WRITE {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}

I get repeated patterns of CHMOD, WRITE then a configuration ALL event using this startup command:

go run main.go start --sources='[{"uri":"config/samples/example_flags.json","provider":"file"}]' --debug 

cmd/start.go Outdated Show resolved Hide resolved
pkg/sync/kubernetes/kubernetes_sync.go Outdated Show resolved Hide resolved
pkg/sync/file/filepath_sync.go Outdated Show resolved Hide resolved
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
Signed-off-by: James Milligan <[email protected]>
@toddbaert
Copy link
Member

toddbaert commented Mar 8, 2023

It's probably unrelated to this change but I'm seeing two WRITE events in the logs every time a flag configuration changes using the file source. Could you please confirm that this change isn't the root cause. If it's not, please create a follow up issue to investigate.

Please can you provide more info - im not able to replicate this behaviour:

2023-03-08T09:27:41.069Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.740Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json CHMOD {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.741Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json WRITE {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:31.741Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json CHMOD {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        info    file/filepath_sync.go:78        filepath event: config/samples/example_flags.json WRITE {"component": "sync", "sync": "filepath"}
2023-03-08T09:28:36.256Z        debug   file/filepath_sync.go:121       Configuration config/samples/example_flags.json:  ALL   {"component": "sync", "sync": "filepath"}

I get repeated patterns of CHMOD, WRITE then a configuration ALL event using this startup command:

go run main.go start --sources='[{"uri":"config/samples/example_flags.json","provider":"file"}]' --debug 

I think this is all old behavior, and not much can be done about it. It seems to be all about how particular editors and programs update files. Even in the most basic cases, programs actually remove/write the file. The only exception is if the file being watched is a symlink and that symlink is updated. That's the only thing that seems to be completely atomic.

Even if I run something like: sed -i 's/"defaultVariant": "on"/"defaultVariant": "off"/g' config/samples/example_flags.json which uses sed's "in-place" option (-i), the file is actually removed and replaced with new contents, resulting in 3 events: CHMOD, REMOVE, WRITE (I had no idea sed worked that way, but apparently it does). I've observed this running on Linux, natively (no docker or WSL involved).

@toddbaert toddbaert self-requested a review March 8, 2023 19:00
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.

Re-tested and re-approved.

@james-milligan james-milligan merged commit 1d80039 into open-feature:main Mar 9, 2023
beeme1mr pushed a commit that referenced this pull request Mar 9, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.4.2](v0.4.1...v0.4.2)
(2023-03-09)


### 🧹 Chore

* Add targeted Flag to example config
([#467](#467))
([6a039ce](6a039ce))
* **deps:** pin dependencies
([#473](#473))
([679e860](679e860))
* **deps:** update google-github-actions/release-please-action digest to
e0b9d18 ([#474](#474))
([5b85b2a](5b85b2a))
* refactoring and improve coverage for K8s Sync
([#466](#466))
([6dc441e](6dc441e))


### 🐛 Bug Fixes

* add registry login
([#476](#476))
([99de755](99de755))
* **deps:** update module golang.org/x/crypto to v0.7.0
([#472](#472))
([f53f6c8](f53f6c8))
* **deps:** update module google.golang.org/protobuf to v1.29.0
([#478](#478))
([f9adc8e](f9adc8e))


### ✨ New Features

* grpc tls connectivity (grpcs)
([#477](#477))
([228f430](228f430))
* introduce per-sync configurations
([#448](#448))
([1d80039](1d80039))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@james-milligan james-milligan deleted the sync-provider-2 branch March 23, 2023 10:58
@github-actions github-actions bot mentioned this pull request Dec 2, 2023
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