Skip to content

[FSSDK-10619] Refactor project config manager to be injectable #945

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 46 commits into from
Sep 27, 2024
Merged

[FSSDK-10619] Refactor project config manager to be injectable #945

merged 46 commits into from
Sep 27, 2024

Conversation

raju-opti
Copy link
Contributor

@raju-opti raju-opti commented Sep 20, 2024 •
edited
Loading

Summary

  • This PR allows the user to create a ProjectConfigManager and pass it directly to createInstance. It exposes two factory methods createStaticProjectConfigManager and createPollingProjectConfigManager for this purpose.

Test plan

  • New tests have been added and relevant old tests has been updated

Issues

  • FSSDK-10619
  • FSSDK-10639
  • FSSDK-10641

@raju-opti raju-opti requested a review from a team as a code owner September 20, 2024 10:09
@raju-opti raju-opti marked this pull request as draft September 20, 2024 10:09
@raju-opti raju-opti changed the title project config manager refactor [DRAFT] project config manager refactor Sep 20, 2024
@raju-opti raju-opti changed the title [DRAFT] project config manager refactor [FSSDK-10619] Refactor project config manager to be injectable Sep 20, 2024
@raju-opti raju-opti marked this pull request as ready for review September 20, 2024 15:46
@coveralls
Copy link

coveralls commented Sep 20, 2024 •
edited
Loading

Coverage Status

coverage: 89.201% (-1.1%) from 90.307%
when pulling c483d85 on raju/pconf
into e7cc602 on master.

@mikechu-optimizely
Copy link
Contributor

Sorry I didn't get to this today. It's first on my list Mon

mikechu-optimizely
mikechu-optimizely approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM.

junaed-optimizely
junaed-optimizely approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@junaed-optimizely junaed-optimizely left a comment

Choose a reason for hiding this comment

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

Great work! So many great additions + modifications.
LGTM!
I just have couple of curious questions.

jaeopt
jaeopt approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM with a few comments.
PR is too big and includes completely rewritten codes, so hard to review. I see no clear errors, but expect all these changes covered by unit tests (and later with FSC).

@raju-opti raju-opti merged commit 1d814a2 into master Sep 27, 2024
11 of 19 checks passed
@raju-opti raju-opti deleted the raju/pconf branch September 27, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raju2592 raju2592 raju2592 left review comments

@jaeopt jaeopt jaeopt approved these changes

@mikechu-optimizely mikechu-optimizely mikechu-optimizely approved these changes

@junaed-optimizely junaed-optimizely junaed-optimizely approved these changes

Assignees

@raju-opti raju-opti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@raju-opti @coveralls @mikechu-optimizely @raju2592 @jaeopt @junaed-optimizely