Open
Conversation
…dates Add StampObservedGeneration helper that sets ObservedGeneration on all conditions to match the object's metadata.generation before every Status().Update() call. This enables consumers to detect stale conditions where the spec changed but the controller hasn't reconciled yet.
…detection Consolidate 6 status files into 3 (status.go, problems.go, response.go) with a SubResourceChecker registry pattern. Add Api as a sub-resource of ApiSpecification, fix silent error dropping, and introduce ObservedGeneration- based staleness detection that downgrades parent processingState from Done to Processing when any sub-resource has stale conditions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves how rover-server derives and reports resource status by (1) stamping ObservedGeneration on conditions during controller status updates and (2) consolidating the status-mapping logic while adding “staleness” detection based on ObservedGeneration vs. metadata.generation.
Changes:
- Stamp
ObservedGenerationon all conditions immediately beforeStatus().Update()in the common controller. - Refactor rover-server status mapping: add staleness detection, split out sub-resource problem/staleness helpers, and adjust mappers to return errors where needed.
- Introduce
ApiStoreand corresponding test mocks to support ApiSpecification sub-resource checks.
Reviewed changes
Copilot reviewed 19 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rover-server/test/mocks/mocks_Api.go | Adds Api store mock wiring (currently depends on a missing api.json fixture). |
| rover-server/test/mocks/mocks.go | Adds apiFileName constant + GetApi testdata loader. |
| rover-server/pkg/store/stores.go | Registers new global ApiStore and initializes it in InitOrDie. |
| rover-server/internal/mapper/status/suite_status_test.go | Initializes store.ApiStore mock for status-mapper tests. |
| rover-server/internal/mapper/status/status_test.go | Updates tests for new MapRoverStatus signature and adds staleness test coverage. |
| rover-server/internal/mapper/status/status.go | Consolidates status logic, adds staleness detection, changes signatures, and moves overall-status helpers here. |
| rover-server/internal/mapper/status/states_test.go | Removes old overall-status tests (moved into status_test.go). |
| rover-server/internal/mapper/status/states.go | Removes old overall-status implementation (moved into status.go). |
| rover-server/internal/mapper/status/stateInfos_test.go | Removes old state-info tests (moved into problems_test.go). |
| rover-server/internal/mapper/status/stateInfos.go | Removes old state-info helpers (replaced by problems.go). |
| rover-server/internal/mapper/status/response.go | Uses generation-aware MapStatus, avoids nil deref for ProcessedAt, and adds stale/sub-resource problem handling. |
| rover-server/internal/mapper/status/resources_test.go | Removes old sub-resource tests (replaced by problems_test.go). |
| rover-server/internal/mapper/status/resources.go | Removes old sub-resource helper implementation (replaced by problems.go). |
| rover-server/internal/mapper/status/problems_test.go | Adds tests for new problems + staleness helper logic. |
| rover-server/internal/mapper/status/problems.go | Introduces unified sub-resource problem collection + staleness checks for Rover and ApiSpecification. |
| rover-server/internal/mapper/status/snapshots/status_test.snap | Updates snapshot for failed->invalid state mapping. |
| rover-server/internal/mapper/rover/out/suite_rover_out_test.go | Initializes mock stores so MapRoverStatus sub-resource checks work in mapper tests. |
| rover-server/internal/mapper/rover/out/rover.go | Adapts to MapRoverStatus returning (status, error). |
| rover-server/internal/mapper/apispecification/out/suite_apispec_out_test.go | Adds ctx + initializes store.ApiStore mock for ApiSpecification mapping. |
| rover-server/internal/mapper/apispecification/out/apispecification_test.go | Updates calls to MapResponse(ctx, ...). |
| rover-server/internal/mapper/apispecification/out/apispecification.go | Makes mapper ctx-aware and uses MapApiSpecificationStatus (now returns error). |
| rover-server/internal/controller/apispecification.go | Threads ctx into out.MapResponse. |
| common/pkg/controller/controller_test.go | Adds unit tests for StampObservedGeneration. |
| common/pkg/controller/controller.go | Stamps ObservedGeneration before status updates + adds StampObservedGeneration helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
lukas016
reviewed
Mar 17, 2026
…mapNotReadyConditionToProblem - Add OverallStatusInvalid (priority 7) and OverallStatusDone (priority 1) to statusPriority so CompareAndReturn handles all valid enum values. Previously, omitted statuses silently received priority 0 from the Go map zero-value, making them less severe than Complete. - Change mapNotReadyConditionToProblem to accept *metav1.Condition instead of a value, avoiding an unnecessary struct copy at the call site. - Add tests for Invalid, Done, and unknown status comparisons.
lukas016
reviewed
Mar 18, 2026
Collaborator
|
LGTM but before approve, pls fix merge conflicts. |
Extend the status pipeline to handle EventSpecification sub-resources (EventType) with staleness detection and problem aggregation, mirroring the existing ApiSpecification flow. Key changes: - Add MapEventSpecificationStatus, MapEventSpecificationResponse, and GetAllEventSpecificationProblems for event spec status reporting - Register EventTypeStore and wire it into both feature-gated and noop store initialization - Extract staleness checkers into dedicated staleness.go for clarity - Generalize the SubResource type constraint to accept any CR instead of enumerating concrete types - Skip store queries when the parent status reports zero sub-resources of a given type (hasRefs guard) - Rename MapResponse to MapRoverResponse / MapApiSpecificationResponse to disambiguate status response mappers - Refactor CompareAndReturn tests to table-driven style
…d Stores struct Convert the rover-server store layer from package-level global variables to an explicit Stores struct that is created once in main and passed through controllers, mappers, and status functions. This eliminates hidden global state and makes dependencies explicit, improving testability and enabling proper parallel test isolation. Key changes: - Replace all package-level store vars with a Stores struct in pkg/store - Thread *Stores through controllers (Rover, ApiSpecification, EventSpecification), mappers (rover/out, apispecification/out, eventspecification/out, applicationinfo), and status functions (staleness, problems, response, status) - Rename identifiers to follow Go initialism conventions (Api -> API) in struct fields, exported functions, mock helpers, and test variables - Decompose FillExposureInfo into fillAPIExposures, fillEventExposures, and fillPublishEventURL to reduce cyclomatic complexity - Skip webhook validation for resources being deleted (ApiSpecification) - Add comprehensive tests for rover in/out mappers, status, staleness, response, applicationinfo, and util packages
…ngle-pass iteration The staleness check and problem collection previously queried the exact same stores and iterated the exact same sub-resources in two separate passes. Merge them into a single pass by adding HasStale and WorstOverallStatus to ProblemsResult, checking isProcessingStale inside getAllProblemsInSubResource. OverallStatus now factors in sub-resource statuses via CompareAndReturn, fixing a bug where it was computed solely from the parent resource. - Delete staleness.go and staleness_test.go (functionality moved into problems.go) - Refactor Map*Status and Map*Response to use single GetAll*Problems call - Add HasStale test cases to problems_test.go - Fix ST1003 naming violations in test variables (Api -> API)
ron96g
requested review from
BjoernKarma,
iagarba,
julius-malcovsky and
stefan-ctrl
BjoernKarma
approved these changes
Mar 23, 2026
Contributor
BjoernKarma
left a comment
BjoernKarma
left a comment
There was a problem hiding this comment.
LGTM
There is one linter issue, but I think we need to have a look and fix all linter issues in all components anyway
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.