Skip to content

Conversation

dmartinol
Copy link
Collaborator

Fixes #2004

  • Split the monolithic Collector interface with SyncStatusCollector and APIStatusCollector to pass only the needed one to the processing methods
  • Use an Error type to transport condition and phase information to the root, instead of a generic error
  • This way, the low-level methods do not need the collectors because they raise the error that carries the desired information

This change is needed to solve #1749 for error paths: some more changes may needed to fix the e2e tests.

@dmartinol dmartinol requested a review from jhrozek September 25, 2025 16:58
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 28.32765% with 210 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.09%. Comparing base (aa8813f) to head (26c3503).

Files with missing lines Patch % Lines
...ator/pkg/mcpregistrystatus/mocks/mock_collector.go 0.00% 77 Missing ⚠️
...thv-operator/controllers/mcpregistry_controller.go 0.00% 65 Missing ⚠️
cmd/thv-operator/pkg/sync/manager.go 33.33% 45 Missing and 1 partial ⚠️
cmd/thv-operator/pkg/registryapi/manager.go 0.00% 17 Missing ⚠️
...thv-operator/pkg/registryapi/mocks/mock_manager.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2016      +/-   ##
==========================================
- Coverage   48.12%   48.09%   -0.03%     
==========================================
  Files         233      235       +2     
  Lines       29229    29281      +52     
==========================================
+ Hits        14066    14083      +17     
- Misses      14131    14168      +37     
+ Partials     1032     1030       -2     

☔ 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.

jhrozek
jhrozek previously approved these changes Sep 28, 2025
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

In general I think this is a very nice refactor, one question about concurrent access inline

- Removed the `Idle` phase from `SyncPhase` and updated related logic to ensure only `Syncing`, `Complete`, and `Failed` phases are used.
- Enhanced the `ShouldSync` method to avoid syncing when the requeue timeout did not elapse.
- Avoid updating sync status at all if sync is not needed (it would cause a new reconciliation)

Signed-off-by: Daniele Martinoli <[email protected]>
@dmartinol
Copy link
Collaborator Author

@jhrozek @yrobla I reviewed the initial implementation again to:

  • remove the Idle sync state (what does it mean? 🤔 ), replaced with Completed
  • avoid syncing if there is any sync state requiring change (e.g. Failed) but before the scheduled requeue time (5m)
  • avoid updating the sync state at all if sync is not needed

The above changes prevent continuous reconciliation loops in case of failures, properly store incremental attempt counter in the sync status and respect the scheduled retries.

Hopefully, the E2E tests will confirm that everything is fine

Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

awesome work @dmartinol

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.

Review the design of Collector interface
3 participants