Remove enable_record_ecosystem_meta feature flag#14353
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the enable_record_ecosystem_meta experiment gating so record_ecosystem_meta is always executed, and updates the updater specs accordingly.
Changes:
- Made
ApiClient#record_ecosystem_metarun unconditionally by removing the experiment early-return. - Simplified
ApiClientspecs by removing experiment stubs and the “feature flag disabled” context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| updater/lib/dependabot/api_client.rb | Removes the experiment guard so ecosystem metadata is always recorded. |
| updater/spec/dependabot/api_client_spec.rb | Removes feature-flag-related stubbing/contexts for ecosystem meta recording. |
|
@copilot review and fix the lint failures as well as the integration test failures |
Fixed both issues in commits
Also cleaned up accidentally committed go_modules fixture files in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
pavera
force-pushed
the
copilot/remove-enable-record-ecosystem-meta-ff
branch
2 times, most recently
from
f6db6c0 to
181ec34
Compare
Lint and Go integration test ( |
The record_ecosystem_meta functionality is now always active (no longer gated behind an experiment flag). Removed: - Feature flag guard in api_client.rb - Associated test stubs and "when feature flag is disabled" test context Co-authored-by: pavera <660677+pavera@users.noreply.github.com>
- Fix RuboCop Layout/EmptyLinesAroundBlockBody offense (extra empty line) - Add stub_request for record_ecosystem_meta_url in spec - Fix Go integration test: check message type before full struct unmarshal so record_ecosystem_meta messages (which have different data structure) are skipped gracefully instead of causing unmarshal errors Co-authored-by: pavera <660677+pavera@users.noreply.github.com>
Remove go.sum/go.mod fixture files that were accidentally picked up by git add and are not related to the feature flag removal. Co-authored-by: pavera <660677+pavera@users.noreply.github.com>
Remove the retry loop from record_ecosystem_meta to avoid adding up to ~30s of extra latency per job when the API call fails. Since this is now unconditional and invoked from multiple ensure blocks, it should be truly best-effort (like increment_metric) with no sleep/retry behavior. Network errors and HTTP 4xx+ responses are logged as errors without retrying. Co-authored-by: pavera <660677+pavera@users.noreply.github.com>
Remove redundant inner rescue for Excon/SSL errors since the outer StandardError rescue already catches those. Single rescue block is cleaner and matches the reviewer's suggested pattern. Co-authored-by: pavera <660677+pavera@users.noreply.github.com>
pavera
force-pushed
the
copilot/remove-enable-record-ecosystem-meta-ff
branch
from
d14f7ba to
7be8284
Compare
pavera
left a comment
pavera
left a comment
There was a problem hiding this comment.
smoke tests need to be updated with the new default output.
| var msg struct { | ||
| Type string `json:"type"` | ||
| } | ||
| if err := json.Unmarshal([]byte(scanner.Text()), &msg); err != nil { | ||
| return nil, fmt.Errorf("failed to decode line %s: %w", scanner.Text(), err) | ||
| } | ||
| if msg.Type != prType { | ||
| continue | ||
| } |
There was a problem hiding this comment.
interesting find... did you spot this bug or had it been silently failing until Copilot spotted it?
There was a problem hiding this comment.
The test failed when we made the default behavior the FF behavior. There wasn't a test previously that exercised this code with the FF enabled so when the output changed, the test started failing.
What are you trying to accomplish?
Remove the
enable_record_ecosystem_metaexperiment flag, makingrecord_ecosystem_metaunconditionally active, and make it truly best-effort (no retries) to avoid adding latency to update jobs.updater/lib/dependabot/api_client.rb: Removed theExperiments.enabled?(:enable_record_ecosystem_meta)early-return guard. Replaced the retry loop (MAX_REQUEST_RETRIESwithsleep) with a single fire-and-forget POST aligned withincrement_metric's best-effort pattern. HTTP 4xx+ responses are logged as errors without retrying. A singleStandardErrorrescue acts as a safety net for any unexpected failures.updater/spec/dependabot/api_client_spec.rb: Removed experiment stubs and the "when feature flag is disabled" test context. Addedstub_requestforrecord_ecosystem_meta_urlso tests validate the actual 204 success path. FixedLayout/EmptyLinesAroundBlockBodylint offense.silent/tests/silent_test.go: Updated the Go integration test to check the messagetypefield before attempting fullCreatePRstruct unmarshal, sorecord_ecosystem_metamessages (which have a differentdatastructure) are skipped gracefully instead of causing unmarshal errors.Anything you want to highlight for special attention from reviewers?
The 8 operation spec files that reference
record_ecosystem_meta: nilin mock doubles are unchanged — those are standardinstance_doublestubs unrelated to the feature flag.The Go integration test (
silent/tests/silent_test.go) needed a fix becauserecord_ecosystem_metais now unconditionally sent in the output stream. Thedatafield inrecord_ecosystem_metamessages is an array of ecosystem objects, not an array ofDependencyFilestructs, so the test now filters by messagetypevia a lightweight preliminary unmarshal before attempting the fullCreatePRstruct decode.The retry loop was removed from
record_ecosystem_metabecause, now that the method is unconditional and invoked from multipleensureblocks, outages could add up to ~30s of extra latency per job. The method is now best-effort with a single POST call, matchingincrement_metric's fire-and-forget behavior. Note thatrecord_cooldown_metastill retains its retry loop — onlyrecord_ecosystem_metawas changed per the review feedback.How will you know you've accomplished your goal?
Existing
record_ecosystem_metatests pass without the experiment stubs, confirming the method executes unconditionally. CI lint and integration tests pass cleanly.Checklist
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.