Repository navigation
refactor(controller): GetChangedTargets takes the entity request and the repository - #335
Merged
justinwon777 merged 1 commit intoOct 15, 2026
Conversation
…the repository The handler already maps the GetChangedTargets request and resolves the repository. It now passes the entity request and the resolved config.RepositoryConfig to the controller, together with the output config and the response stream. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for its cache keys and its phase metrics, and the repository configuration for its seed attributes. It still filters and sends to the stream. The handler no longer carries the comment that names its mapping as temporary for this RPC. The Controller interface method changes, and the mock is regenerated. The controller test TestGetChangedTargets_ValidationError goes: it passed a nil proto request, so it tested the conversion that the handler owns, and the mapper tests cover that conversion. The controller keeps its repository resolver, controller.Params.RepoConfig, and their test in this commit. The next commit removes them. Test plan: - make build, make test, make lint - handler tests check the forwarded entity request, output config, stream, and repository - controller tests build the entity request and the repository at each call site and call GetChangedTargets directly Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This was referenced Oct 15, 2026
justinwon777
marked this pull request as ready for review
October 15, 2026 09:50
justinwon777
added this pull request to stack #337
October 15, 2026 09:51
yushan8
approved these changes
Oct 15, 2026
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
**Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler (this PR) 2. #334 `GetTargetGraph` takes the entity request and the repository 3. #335 `GetChangedTargets` takes the entity request and the repository 4. #336 Remove the repository resolver ## What - The handler maps the request, resolves the repository, and tags the metrics with the repository ID. A bad request or an unconfigured remote gets the tag `unknown` and never reaches the controller. - The controller keeps its phase metrics (cache lookups, durations, `target_count`) and `send_duration`. - `start` and `finish` now publish under `handler.<operation>` instead of `controller.<operation>`. Names, tags, and counts do not change. ## Temporary duplication The handler and the controller both map the request and resolve the repository. The controller needs the result for cache keys. A follow-up change passes the entity request and the resolved repository to the controller and removes the duplicate. The duplicate code has comments that say so. ## Test plan: - make build, make test, make lint - handler tests cover the lifecycle metrics of each RPC with the repo tag ## Issue <!-- Link the issue here. - Use 'Closes #123' if this is the final fix. - Use 'Part of #123' or just '#123' if the feature is still in progress. --> #325 Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
… repository (#334) **Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler 2. #334 `GetTargetGraph` takes the entity request and the repository (this PR) 3. #335 `GetChangedTargets` takes the entity request and the repository 4. #336 Remove the repository resolver Depends on #333. Review and merge in order. ## What The handler passes the entity request and the resolved `config.RepositoryConfig` to `GetTargetGraph` in the controller. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for cache keys and phase metrics. It still filters and sends to the stream. The `Controller` interface no longer embeds the generated server, because its first method now takes entity types. It lists `GetTargetGraph` and `GetChangedTargets`. The stub `GetChangedTargetGraph` leaves the controller. The handler serves it: it starts the metrics op, returns the Unimplemented infra error, and completes the op. It makes no controller call. The controller keeps its resolver for `GetChangedTargets` only. ## Test plan - make build, make test, make lint - Handler tests check the forwarded entity request, output config, stream, and repository. - Controller tests call `GetTargetGraph` with the entity request. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
justinwon777
added a commit
that referenced
this pull request
Oct 15, 2026
**Stack** (each PR is based on the one before it): 1. #333 Move the metrics lifecycle to the handler 2. #334 `GetTargetGraph` takes the entity request and the repository 3. #335 `GetChangedTargets` takes the entity request and the repository 4. #336 Remove the repository resolver (this PR) Depends on #335. Review and merge in order. ## What The controller no longer uses its repository resolver, because both RPCs take the resolved repository from the handler. This PR removes the dead code: - `resolveRequestRepository`, `unknownRepositoryMetricLabel`, `controller.Params.RepoConfig`, and the `repoConfig` field of the controller - the controller test of the resolver (the handler has its own test of the same function) - the `RepoConfig` setting on `controller.Params` in tests, `example`, and the integration test - the handler comments that named its copies as temporary ## Breaking change for callers Callers that build `controller.Params`, such as go-code, must stop setting `RepoConfig`. They must set it on `handler.Params` instead. ## Test plan - make build, make test, make lint 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
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
2 participants
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.
Stack (each PR is based on the one before it):
GetTargetGraphtakes the entity request and the repositoryGetChangedTargetstakes the entity request and the repository (this PR)Depends on #334. Review and merge in order.
What
The handler passes the entity request and the resolved
config.RepositoryConfigtoGetChangedTargetsin the controller. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for cache keys and phase metrics, and the repository configuration for seed attributes. It still filters and sends to the stream.The controller test
TestGetChangedTargets_ValidationErrorgoes. It passed a nil proto request, so it tested the conversion that the handler owns. The mapper tests cover that conversion.The controller keeps its resolver,
controller.Params.RepoConfig, and their test in this PR. The next PR removes them.Test plan
GetChangedTargetsdirectly.🤖 Generated with Claude Code