Added initial implementation of kosli snapshot cloud-run server/#4986

[ToreMerkely]

• feat(snapshot): add hidden cloud-run skeleton command (#4986)
• feat(cloudrun): add internal/cloudrun package wrapping the Cloud Run Admin API (#4986)
• feat(snapshot): wire cloud-run command end-to-end via internal/cloudrun (#4986)
• feat(snapshot): add service filtering flags to cloud-run command (#4986)
• feat(cloudrun): friendly auth and not-found error messages (#4986)
• fix(cloudrun): align snapshot payload with server's snapshot-examples doc (#4986)

[claude]

Claude finished @mbevc1's task in 5m 4s —— View job

---

PR Review: kosli snapshot cloud-run

• Phase 1: Static analysis (go build, go vet, go mod tidy)
• Phase 2: Read and review all changed files
• Phase 3: Post inline comments and summary

---

Static Analysis

All three checks pass cleanly: go build ./..., go vet ./..., and go mod tidy with no drift in go.mod/go.sum.

---

Summary

This is a well-structured PR that adds kosli snapshot cloud-run as a hidden, dry-run-only command across 6 incremental slices. The code closely follows the established ECS snapshot pattern, which makes it easy to review and maintain.

What needs attention (1 actionable item):

1. gRPC client resource leak — run.NewServicesClient and run.NewRevisionsClient open gRPC connections that are never closed. If NewRevisionsClient fails, the already-created services client also leaks. Add a Close() method to Client and defer client.Close() in the command. See inline comment for the suggested fix. Both @mbevc1 and the prior automated review flagged this — it's the one thing I'd address before merging.

Minor observations (no action required):

• The trafficRevisionNames fallback to LatestReadyRevision when Revision is empty works correctly per the API contract, but an explicit GetType() == LATEST check would make intent clearer. See inline.
• The filtered := services[:0] slice reuse (line 98 of snapshotCloudRun.go) is correct but a subtle idiom — the prior review already flagged this as a readability nit.
• N+1 API calls per service (one GetRevision per traffic target) is a known trade-off documented in the handover doc — fine for now.

What's done well:

• Clean vertical slicing — each of the 6 commits is independently coherent and reviewable. The handover doc tracks decisions and rationale clearly.
• Thorough test coverage — 9 unit tests in internal/cloudrun covering traffic splits, dedup, LATEST resolution, multi-container, tag-pinned images, and zero-traffic edge cases. 13 command-level tests covering arg validation, flag mutual exclusion, filtering (include/exclude by name and regex), and auth error UX.
• Good use of existing patterns — filters.ResourceFilterOptions, cmdTestCase table tests, cloudRunLister interface seam for testability, and Classify for friendly error messages all follow established codebase conventions.
• Defensive design — hidden + forced dry-run until the feature is complete, EnvRequest.Artifacts initialized as non-nil empty slice, Classify preserves the original error via %w for unwrapping.
• Payload alignment — the out-snapshot-examples.txt reference was followed carefully, and the Slice 3.5 correction (dropping project/region per artifact to avoid extra="forbid" rejection) shows good attention to the server contract.

Overall this is solid work. The gRPC close is the only must-fix; everything else is minor or already tracked for future slices.

[claude]

Resource leak — GCP gRPC clients are never closed.

run.NewServicesClient and run.NewRevisionsClient open gRPC connections. The current code never calls .Close() on them. For a short-lived CLI command this isn't critical (the OS reclaims on exit), but it's best practice and avoids noisy warnings from the gRPC library.

Consider adding a Close method to Client (and to the apiClient interface or alongside it) so the caller can defer client.Close():

Suggested change

	func New(ctx context.Context) (*Client, error) {
	services, err := run.NewServicesClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	revisions, err := run.NewRevisionsClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	return &Client{api: &gcpAPI{services: services, revisions: revisions}}, nil
	}
	func New(ctx context.Context) (*Client, error) {
	services, err := run.NewServicesClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	revisions, err := run.NewRevisionsClient(ctx)
	if err != nil {
	services.Close()
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	return &Client{api: &gcpAPI{services: services, revisions: revisions}}, nil
	}
	// Close releases the underlying gRPC connections.
	func (c *Client) Close() error {
	if g, ok := c.api.(*gcpAPI); ok {
	sErr := g.services.Close()
	rErr := g.revisions.Close()
	if sErr != nil {
	return sErr
	}
	return rErr
	}
	return nil
	}

Note the second change: if NewRevisionsClient fails, the already-created services client should be closed to avoid leaking that connection too.

Then in the command: defer client.Close() after the newCloudRunClient call.

[mbevc1]

Adding Close is a fair comment

[claude]

Nit: in-place slice filtering is correct but non-obvious.

filtered := services[:0] reuses the backing array of services. It's safe here because range copies each element before the append overwrites earlier positions, and services is never read again. But it's a subtle idiom — a plain var filtered []cloudrun.Service (or make([]cloudrun.Service, 0, len(services))) would be immediately clear with no mental overhead, and the allocation cost is negligible for the expected number of services.

Not a bug, just a readability suggestion.

[claude]

Consider checking t.GetType() explicitly for LATEST resolution.

Currently, any traffic target with an empty Revision field falls through to LatestReadyRevision. This works because the Cloud Run API contract guarantees only LATEST targets have an empty revision — but an explicit type check would make the intent self-documenting and guard against future API changes:

name := t.GetRevision()
if t.GetType() == runpb.TrafficTargetAllocationType_TRAFFIC_TARGET_ALLOCATION_TYPE_LATEST || name == "" {
    name = shortName(svc.GetLatestReadyRevision())
}

Minor defensive improvement — not a bug today.

[claude]

Package-level mutable var for test seam — works but has a known trade-off.

This pattern (save/restore in SetupTest/TearDownTest) is correct for sequential test execution, but if tests ever run in parallel (t.Parallel()), the shared mutable global would race. Since cmd/kosli tests use suite.Suite and don't parallelize, this is fine today. Just flagging for awareness — the testify suite pattern naturally serializes, so no action needed unless the test approach changes.

[claude]

N+1 API calls per service (ListServices + GetRevision per traffic target).

For each service, the code makes one GetRevision call per traffic target. With many services or multi-revision traffic splits, this could add up. The handover doc already notes this:

"services excluded by name still cost their revision-fetch round-trips. If that becomes a bottleneck, push the filter into cloudrun.ListServices."

Good that it's tracked. For a future slice, consider batching or pushing the name filter into ListServices so excluded services skip the per-revision API calls entirely.

[claude]

Digests will serialize as null when the revision has no containers.

If rev.Digests is nil (e.g. a revision with zero containers), the JSON field becomes "digests": null rather than "digests": {}. Depending on the server's validation, this might be rejected. The toRevision function in cloudrun.go:127 does make(map[string]string, len(rev.GetContainers())) which returns an empty (non-nil) map for zero containers, so this is fine in practice from the current code path. But if RevisionData is ever constructed directly with a nil Digests, it could surprise.

No action needed now — just noting the invariant to preserve.

[claude]

Agreeing with the earlier review and @mbevc1's comment — the gRPC clients should be closed. Also, if NewRevisionsClient fails, the already-created services client leaks.

Suggested change

	func New(ctx context.Context) (*Client, error) {
	services, err := run.NewServicesClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	revisions, err := run.NewRevisionsClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	return &Client{api: &gcpAPI{services: services, revisions: revisions}}, nil
	}
	func New(ctx context.Context) (*Client, error) {
	services, err := run.NewServicesClient(ctx)
	if err != nil {
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	revisions, err := run.NewRevisionsClient(ctx)
	if err != nil {
	services.Close()
	return nil, fmt.Errorf("GCP client setup failed: %w", err)
	}
	return &Client{api: &gcpAPI{services: services, revisions: revisions}}, nil
	}
	// Close releases the underlying gRPC connections.
	func (c *Client) Close() error {
	if g, ok := c.api.(*gcpAPI); ok {
	sErr := g.services.Close()
	rErr := g.revisions.Close()
	if sErr != nil {
	return sErr
	}
	return rErr
	}
	return nil
	}

Then in snapshotCloudRun.go after newCloudRunClient(ctx), add a defer client.Close() (which means cloudRunLister needs a Close() error method, or add the defer with a type-assert).

[claude]

Once Close() is added to the client, add a defer here:

client, err := newCloudRunClient(ctx)
if err != nil {
    return err
}
if closer, ok := client.(io.Closer); ok {
    defer closer.Close()
}

Or add Close() error to cloudRunLister directly — it's a private interface, so that's fine. The stub can return nil.

[claude]

Nit: unbounded growth pattern. If a project has many services, out will grow via repeated append. Since you already have len(rawServices) above, pre-allocating is fine there, but for listServices you could consider using the iterator's page size hint. Not a real issue for typical Cloud Run deployments (tens of services), just noting.

[claude]

Consider what happens when services is nil vs empty:

• ToEnvRequest(nil) → Artifacts is [] (non-nil, due to the []*RevisionData{} literal) — good, serializes as "artifacts": []
• ToEnvRequest([]Service{}) → same — good

This is correctly handled. Nice.

[claude]

The dedup logic is clean, but as the earlier review noted, an explicit t.GetType() == LATEST check would make the fallback to LatestReadyRevision self-documenting rather than relying on the empty-revision contract. Low priority since the API contract is stable, but worth considering.

Also: if LatestReadyRevision is itself empty (e.g., a brand-new service that hasn't had a successful deployment), the name == "" check on line 114 correctly skips it — good edge case handling.