Skip to content

fix: implement requirement 1.6.2 #400

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

Merged
merged 1 commit into from
Aug 18, 2025

Conversation

bduffany
Copy link
Contributor

@bduffany bduffany commented Jun 28, 2025

Implement requirement 1.6.2 by having the Shutdown function shut down only the registered providers and clean up all resources.

Fixes #397

Copy link

codecov bot commented Jun 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.69%. Comparing base (5b0218b) to head (7eb06d7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #400   +/-   ##
=======================================
  Coverage   85.68%   85.69%           
=======================================
  Files          20       20           
  Lines        1432     1433    +1     
=======================================
+ Hits         1227     1228    +1     
  Misses        184      184           
  Partials       21       21           
Flag Coverage Δ
e2e 85.69% <100.00%> (+<0.01%) ⬆️
unit 85.69% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bduffany bduffany force-pushed the shutdown-fix branch 5 times, most recently from fa3fc6f to 7a783dd Compare June 29, 2025 00:33
@toddbaert
Copy link
Member

I'll take a look at this next week.

@toddbaert
Copy link
Member

I hope the specification enhancement here makes the way forward here less ambiguous.

@bduffany bduffany changed the title fix: only shut down active providers fix: implement requirement 1.6.2 Aug 15, 2025
@bduffany bduffany changed the title fix: implement requirement 1.6.2 fix: partially implement requirement 1.6.2 Aug 15, 2025
@bduffany bduffany changed the title fix: partially implement requirement 1.6.2 fix: implement requirement 1.6.2 Aug 15, 2025
The spec says that shutdown must shut down active providers,
but the current logic shuts down inactive providers.

This change fixes that. Separately, the spec might also need to be modified
to say "only" active providers.

Signed-off-by: Brandon Duffany <[email protected]>
@sahidvelji sahidvelji enabled auto-merge (squash) August 15, 2025 21:35
@sahidvelji sahidvelji merged commit a9c3adc into open-feature:main Aug 18, 2025
6 checks passed
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.

[BUG] Inactive providers are shut down more than once
5 participants