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 Feature Flags module to Ligare.web and Ligare.platform #107

Merged
merged 42 commits into from
Oct 4, 2024

Conversation

aholmes
Copy link
Member

@aholmes aholmes commented Sep 12, 2024

This PR contains refactors and feature additions to support adding a DI module to Ligare.platform that can handle the logic needed for Feature Flag usage, and the addition of an ASGI module to support a Feature Flag GET/PATCH API in Ligare.web.

@aholmes aholmes force-pushed the aholmes-update-feature-flags branch 2 times, most recently from a3aaebf to a05d6ca Compare September 18, 2024 18:30
@aholmes aholmes force-pushed the aholmes-update-feature-flags branch from dc63f31 to 8e525d5 Compare September 27, 2024 00:15
Moved over existing code from CAP and abstracted feature flag classes
into generics. This allows us to get all, or filtered, feature flags in
a type-safe way.
correctly in `get_feature_flags`.

Also added clearer docblocks to feature flag functions.
Also removed `TAppConfig` generics from application builder.
This requires some updates to how the Flask client creator for tests
works. This commit contains an intentionally broken test.
- previously unable to run `make test*` or `make format*` targets due to
  empty variable value for `BUILD_TARGET`
- `make test-pytest` did not exit with an error code when `pytest` had
  failed tests
@aholmes aholmes force-pushed the aholmes-update-feature-flags branch from bd283c8 to 6329907 Compare September 30, 2024 17:37
application configs and modules to init methods used in tests.

This changes allows tests to module configs and modules that would
otherwise be included in the test app. Some tests may need to operate
without certain modules.
@aholmes aholmes marked this pull request as ready for review October 2, 2024 16:01
@aholmes aholmes requested a review from a team as a code owner October 2, 2024 16:01
@aholmes aholmes changed the title Feature flags Add Feature Flags module to Ligare.web Oct 2, 2024
@aholmes aholmes changed the title Add Feature Flags module to Ligare.web Add Feature Flags module to Ligare.web and Ligare.platform Oct 2, 2024
@aholmes aholmes force-pushed the aholmes-update-feature-flags branch from fb5eaca to 62e47bd Compare October 4, 2024 21:15
@dataclass(frozen=True)
class FeatureFlagChange:
name: str
old_value: bool | None

Choose a reason for hiding this comment

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

Is it necessary to have both old and new values here? What do we gain from storing the old value, and is there a risk if this gets out of sync? (Say, with multiple requests)

Copy link
Member Author

Choose a reason for hiding this comment

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

We only use this in the HTTP PATCH response as a means to report back the old and new values. Because the status code is the same regardless of whether a value did change, this is a means for the caller to determine whether the value actually changed.

Copy link

@dan-knight dan-knight left a comment

Choose a reason for hiding this comment

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

LGTM from a code perspective. Changes seem to stay within existing patterns, so I'm trusting the tests here. I may have some design suggestions once I start using this with a real application, but I don't feel there's too much I can comment on until then.

@aholmes
Copy link
Member Author

aholmes commented Oct 4, 2024

LGTM from a code perspective. Changes seem to stay within existing patterns, so I'm trusting the tests here. I may have some design suggestions once I start using this with a real application, but I don't feel there's too much I can comment on until then.

I'm resolving some scaffolding bugs, but once those are complete we can use the scaffolded applications as reference implementations. Should make it easier to go off of than relying on CAP, since that mixes in a bunch of domain concerns as well.

@aholmes aholmes merged commit b015d00 into main Oct 4, 2024
12 checks passed
@aholmes aholmes deleted the aholmes-update-feature-flags branch October 4, 2024 22:58
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