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.

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

Refactor feature flag overrider architecture to make use of the manager pattern #85

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Refactor feature flag overrider architecture to make use of the manager pattern #85

merged 2 commits into from
Jun 19, 2024

Conversation

jeremynikolic
Copy link
Contributor

No description provided.

@jeremynikolic jeremynikolic self-assigned this Jun 12, 2024
@jeremynikolic jeremynikolic force-pushed the feat_overrider_refactor branch 5 times, most recently from 3ae9c56 to 6762c33 Compare June 12, 2024 13:53
owenvoke
owenvoke reviewed Jun 12, 2024
View reviewed changes
Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

Looks great in my opinion. 👍🏻 Would be good to do some testing platform with a PR to ensure everything transitions smoothly.

Only things I've found should be resolved with an ecs --fix (except the comment I made, which needs to be done manually).

src/Overriders/InMemoryOverrider.php Outdated Show resolved Hide resolved
@jeremynikolic
Copy link
Contributor Author

jeremynikolic commented Jun 13, 2024 •
edited
Loading

@owenvoke Platform would require small adjustments so I was planning to do a PR there yes as you suggested👍
What would be best for that ?
should I create a "test" tag pointing on this PR and have the platform PR point to it for testing ?

@owenvoke
Copy link
Member

owenvoke commented Jun 13, 2024 •
edited
Loading

@jeremynikolic, if you add the branch as the version, it should all work fine. 👍🏻

...
"worksome/feature-flags": "dev-feat_overrider_refactor"
...

Happy to pair or assist if needed.

@jeremynikolic
Copy link
Contributor Author

@owenvoke wonderful, I did not know you could do that :D

@jeremynikolic jeremynikolic force-pushed the feat_overrider_refactor branch 3 times, most recently from 87fe6dc to c598a7e Compare June 13, 2024 11:28
@jeremynikolic jeremynikolic force-pushed the feat_overrider_refactor branch 9 times, most recently from 621a2fc to 3d238e9 Compare June 14, 2024 07:31
owenvoke
owenvoke reviewed Jun 14, 2024
View reviewed changes
src/FeatureFlagsApiManager.php Show resolved Hide resolved
src/Overriders/InMemoryOverrider.php Outdated Show resolved Hide resolved
@jeremynikolic jeremynikolic force-pushed the feat_overrider_refactor branch 2 times, most recently from 9050990 to a6dd040 Compare June 14, 2024 09:12
feat: Make overriders definition more extendable
aed2170 
Adjust the way config works around overriders so that other overriders may be defined outside the package using the manager pattern
@jeremynikolic jeremynikolic marked this pull request as ready for review June 14, 2024 09:24
@jeremynikolic
Copy link
Contributor Author

@owenvoke do you feel we are in a good place now with this and the PR to platform to merge ? 🤔

@owenvoke
Copy link
Member

@jeremynikolic, I've just pushed some ECS changes. But otherwise LGTM for a 2.x release!

@jeremynikolic
Copy link
Contributor Author

Nice thanks @owenvoke
I'll look into merging this one and adjusting the other with proper version 👌

@jeremynikolic
Copy link
Contributor Author

@owenvoke if you may approve then 😄

@owenvoke
Copy link
Member

Whoops my bad.

@jeremynikolic jeremynikolic merged commit 7479bdc into main Jun 19, 2024
11 checks passed
@jeremynikolic jeremynikolic deleted the feat_overrider_refactor branch June 19, 2024 20:53
@owenvoke
Copy link
Member

We'll also need to update the docs where necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owenvoke owenvoke owenvoke approved these changes

Assignees

@jeremynikolic jeremynikolic

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeremynikolic @owenvoke