Skip to content

refactor(controller): remove the repository resolver - #336

Merged
justinwon777 merged 1 commit into
justin.won/handler-entity-changedtargetsfrom
justin.won/handler-remove-resolver
Oct 15, 2026
Merged

refactor(controller): remove the repository resolver#336
justinwon777 merged 1 commit into
justin.won/handler-entity-changedtargetsfrom
justin.won/handler-remove-resolver

Conversation

@justinwon777

@justinwon777 justinwon777 commented Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stack (each PR is based on the one before it):

  1. refactor(handler): move the metrics lifecycle to the handler #333 Move the metrics lifecycle to the handler
  2. refactor(controller): GetTargetGraph takes the entity request and the repository #334 GetTargetGraph takes the entity request and the repository
  3. refactor(controller): GetChangedTargets takes the entity request and the repository #335 GetChangedTargets takes the entity request and the repository
  4. refactor(controller): remove the repository resolver #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

@justinwon777 @claude
refactor(controller): remove the repository resolver
5b3c99e 
The controller no longer uses its repository resolver, because both RPCs now take the resolved repository from the handler. This commit removes the dead code: resolveRequestRepository, unknownRepositoryMetricLabel, controller.Params.RepoConfig, and the repoConfig field of the controller. The controller test of the resolver goes with it, because the handler has its own test of the same function. The tests and the wiring in example and integration stop setting RepoConfig on controller.Params. The handler drops the comments that named its copies of the constant and the resolver as temporary.

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

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This was referenced Oct 15, 2026
Merged
Merged
Merged
@justinwon777
justinwon777 marked this pull request as ready for review October 15, 2026 09:50
@justinwon777
justinwon777 requested review from a team as code owners October 15, 2026 09:50
@justinwon777
justinwon777 added this pull request to stack #337 October 15, 2026 09:51
@justinwon777
justinwon777 merged commit 21d2315 into main Oct 15, 2026
16 checks passed
justinwon777 added a commit that referenced this pull request Oct 15, 2026
@justinwon777 @claude
refactor(handler): move the metrics lifecycle to the handler (#333)
8f77395 
**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
@justinwon777 @claude
refactor(controller): GetTargetGraph takes the entity request and the…
5589d54 
… 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
@justinwon777 @claude
refactor(controller): GetChangedTargets takes the entity request and …
c90dece 
…the repository (#335)

**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
(this PR)
4. #336 Remove the repository resolver

Depends on #334. Review and merge in order.

## What

The handler passes the entity request and the resolved
`config.RepositoryConfig` to `GetChangedTargets` 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,
and the repository configuration for seed attributes. It still filters
and sends to the stream.

The controller test `TestGetChangedTargets_ValidationError` goes. 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

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

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yushan8 yushan8 yushan8 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@justinwon777 @yushan8