chore(flags): remove dead FlagError variants#61000
Conversation
|
Hey @haacked! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
|
Reviews (1): Last reviewed commit: "Merge branch 'master' into fix/flag-erro..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR cleans up the feature-flags Rust service error surface by removing FlagError enum variants that were no longer constructible anywhere in the repository, along with their now-unreachable handling and associated test matrix entries. This reduces boilerplate in preparation for the broader error-handling refactor described in #30174.
Changes:
- Removed five dead
FlagErrorvariants and their match arms inerror_metadata()/IntoResponse, plus related test vectors. - Updated canonical logging tests and error test matrices to reflect the reduced variant set (and to include
HashKeyOverrideErrorin the canonical log test matrix).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rust/feature-flags/src/handler/canonical_log.rs | Updates the parameterized canonical log test cases to remove deleted errors and include HashKeyOverrideError. |
| rust/feature-flags/src/api/errors.rs | Removes dead FlagError variants and their response/metadata handling; updates test vectors accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gustavohstrassburger
left a comment
There was a problem hiding this comment.
Trust stamp
Five variants are never constructed anywhere in the codebase: - DeserializeFiltersError - NoGroupTypeMappings - GroupTypeMappingFetchFailed - PropertiesNotInCache - StaticCohortMatchesNotCached Removing them along with their arms in error_metadata, is_5xx, IntoResponse, and parameterized test matrices. Preparatory cleanup for #30174 before the InternalError/Unavailable bucket collapse in a follow-up PR.
HashKeyOverrideError is a live 5xx variant but was missing from the test_error_codes_are_non_empty, test_error_codes_are_unique, test_error_code_consistency_with_is_5xx vecs, and the canonical_log.rs parameterized case matrix. Adding it closes a pre-existing coverage gap while the vectors are already open from the dead-variant removal above. Per @haacked's review on #55115.
33f75c5 to
142ce96
Compare
Problem
rust/feature-flags/src/api/errors.rshas accumulatedFlagErrorvariants that are never constructed anywhere in the codebase. They exist only in the enum definition, theerror_metadata()/is_5xx()/IntoResponsearms, and test matrices. They add boilerplate flagged in #30174 without carrying any runtime meaning.Part of #30174.
Note
This re-opens @ricardothe3rd's contribution from #55115 on an internal branch. Fork PRs can't read the Depot build token, so the
build-imagesjobs fail withpermission_deniedbefore any code is validated. Running from an internal branch lets the full CI suite execute. The change is entirely @ricardothe3rd's work and their commits are preserved unmodified.Changes
Removes 5 variants that a full-workspace grep and LSP check confirmed have zero construction sites outside
errors.rsand its test modules:DeserializeFiltersErrorNoGroupTypeMappingsGroupTypeMappingFetchFailedPropertiesNotInCacheStaticCohortMatchesNotCachedAlso removes each variant's arm in
error_metadata(),is_5xx(), andIntoResponse, the#[case]entries in the parameterizedcanonical_log.rstest, and the test vectors inerrors.rs.Net 77 deletions, 1 insertion across two files. There is no behavior change because the deleted code paths were unreachable in production.
Scoping note
This is preparatory cleanup for #30174. It does not yet introduce the
InternalError(anyhow::Error)/Unavailable(anyhow::Error)buckets. That collapse is left to a follow-up so this PR stays a clean, low-risk, independently mergeable first step.How did you test this code?
Verification from the original contribution, covering the cargo gates rather than a running service:
cargo check -p feature-flags, clean compilecargo test -p feature-flags --lib api::errors, 14/14 passcargo test -p feature-flags --lib handler::canonical_log, 78/78 passcargo clippy -p feature-flags --all-targets, no new warningscargo fmt -p feature-flags --check, cleanerror_code()string, zero references outside the two edited filesIntegration tests are left to CI, which has the pre-configured Django and ClickHouse environment.
Automatic notifications
Docs update
No docs changes needed.
Co-authored-by: Ricardo rarganaiii@gmail.com
Fixes: #55115