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

Add test for synchronous replication #135

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

rg2011
Copy link
Contributor

@rg2011 rg2011 commented Mar 9, 2022

This PR adds a new test for resource replication, based on client-go FakeDynamicClient to simulate a connection to an actual cluster. The test checks that kube-mgmt reads the resources in this fake cluster and pushes them to a mocked OPA data interface.

Unfortunately, FakeDynamicClient brings in a few new dependencies (github.com/pkg/errors, github.com/evanphx/json-patch) and vendors a few more libs. But I think it's worth adding the test, to address confidently some pending issues related to resource sync that I intend to fix (#11, #134)

To ease testing, I have modified data.New to accept the fake dynamic.Interface as a parameter, instead of a *rest.Config, which incidentally also fixes issue #99.

I have also refactored generateSyncPayload and objPath as functions instead of methods, and made sync.Run receive a context.Context instead of returning one quit chan struct{} per resource, for the sake of easier testing.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Some nitpicks, but nothing that can't be ignored; just stylistic preferences.

I'm uncertain about the API compatibility; do you, or does anyone, have any insights into that...?

pkg/data/generic.go Outdated Show resolved Hide resolved
pkg/data/mock.go Outdated Show resolved Hide resolved
pkg/data/mock.go Outdated Show resolved Hide resolved
@rg2011 rg2011 force-pushed the feature/sync-test branch 3 times, most recently from e9c29a8 to a30cbe1 Compare March 9, 2022 13:12
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Something seems to be slightly odd with your committer email vs github account settings: github still thinks your a first-time contributor. Is that something you care about? Otherwise, this looks good to go.

@rg2011 rg2011 force-pushed the feature/sync-test branch from a30cbe1 to c572357 Compare March 9, 2022 14:21
@rg2011
Copy link
Contributor Author

rg2011 commented Mar 9, 2022

Thanks for pointing out. I've reviewed my github settings and pushed again with the proper user and email (I hope). Let's see if it makes a difference. Otherwise never mind, let's go with with this PR as it is and I'll try to figure out later.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks

@srenatus srenatus merged commit c71de33 into open-policy-agent:master Mar 9, 2022
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.

2 participants