Skip to content

Added part two of cloud run job reporting server/#4986#867

Merged
ToreMerkely merged 5 commits into
mainfrom
4986-cloud-run-job-reporting-2
May 8, 2026
Merged

Added part two of cloud run job reporting server/#4986#867
ToreMerkely merged 5 commits into
mainfrom
4986-cloud-run-job-reporting-2

Conversation

@ToreMerkely
Copy link
Copy Markdown
Contributor

  • feat(cloudrun): list Cloud Run Jobs via JobsClient (Slice 2, #4986)
  • feat(cloudrun): emit Cloud Run Jobs in env snapshots (Slice 3, #4986)

ToreMerkely and others added 2 commits May 8, 2026 08:37
Add the internal primitives for surfacing Cloud Run Jobs alongside
Services in env snapshots. Jobs do not have a revision/traffic-split
model — there is one current image per Job, taken from
Template.Template.Containers in the Job resource.

  - New `Job` struct (Name, Digests, CreatedAt) mirroring `Revision`.
  - `apiClient` interface gains `listJobs`; production `gcpAPI` uses
    `run.NewJobsClient` and the same iterator pattern as listServices.
  - `Client.ListJobs(ctx, project, region)` returns `[]Job`.
  - `New()` constructs the JobsClient and cleans up the existing two
    clients on construction error. `Close()` joins all three.
  - Unlike Services, ListJobs returns the full Job (with Template
    populated), so no per-resource follow-up call is needed.

No caller wires this up yet — that's Slice 3 (cmd layer + snapshot
payload). Tests cover empty input, digest-pinned and tag-pinned
images, multi-container task templates, multi-job listings, nil
CreateTime, and error propagation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire ListJobs end-to-end so idle Cloud Run Jobs surface in
\`kosli snapshot cloud-run\` alongside Service revisions. The whole
point of #4986: a Cloud Run Job's image lives on the Job resource
itself, not on a running Execution, so batch / scheduled / event-
driven Jobs that spend most of their time idle were previously
invisible.

  - \`cloudRunLister\` interface gains ListJobs.
  - \`run()\` lists Jobs alongside Services and applies the same name
    filter to both, so existing --services / --exclude / -regex flags
    work uniformly across kinds (the kosli-reporter self-reference
    filter that the test bed relies on continues to work via
    --exclude kosli-reporter).
  - \`ToEnvRequest(services, jobs)\` emits service artifacts first, then
    one artifact per job (kind=job, jobName).
  - Success log "[N] revisions were reported" → "[N] artifacts were
    reported" since the count now mixes revisions and jobs.

Tests cover: dry-run output containing both kinds, the live-server
happy-path with 2 services + 1 job → 3 artifacts, and the filter
applying to job names.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ToreMerkely ToreMerkely enabled auto-merge (squash) May 8, 2026 06:55
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 8, 2026

Claude finished @ToreMerkely's task in 1m 28s —— View job


PR Review: Cloud Run Job Reporting (Slices 2–6, #4986)

  • Phase 1: Static analysis (build, vet, mod tidy)
  • Phase 2: Read and review changed files
  • Phase 3: Post review feedback

Static Analysis

All three checks pass cleanly — go build ./..., go vet ./..., and go mod tidy report no issues.


Summary

This is a well-structured, clean PR. The five slices follow the repo's TDD/thin-vertical-slice conventions closely: each commit adds one capability, the codebase stays green throughout, and the test coverage is thorough. I have no blocking issues.

What's Done Well

  • Telescoping cleanup in New() (internal/cloudrun/cloudrun.go:66-83): The constructor properly closes already-opened clients on partial failure. If NewJobsClient fails, both services and revisions get closed. This pattern was extended correctly from the two-client version.
  • toJob mirrors toRevision exactly — same nil-CreateTime guard, same parseDigest call, same shortName extraction. No divergence to worry about.
  • Flag rename from --services/--services-regex to --include/--include-regex is a smart move while the command is still hidden/pre-release — no backwards-compat burden.
  • Payload tests lock the wire format (TestToEnvRequest_JobSerializesAsFlatJobName, TestToEnvRequest_ServicesAndJobsMixed) — these will catch accidental regressions in the JSON shape sent to the server.
  • Filter tests exercise both inclusion and exclusion on jobs (TestSnapshotCloudRunFilter_AppliesToJobs), confirming the uniform filter behaviour the help text promises.
  • listJobs error test (TestListJobs_ListErrorPropagates) in cloudrun_test.go with a separate listJobsErr field on fakeAPI — more granular than the command-level stub, and the right place for it.

Minor Observations (Non-blocking)

  1. SetupTest default stub doesn't include jobs (snapshotCloudRun_test.go:95-97): The default newCloudRunClient in SetupTest returns stubCloudRunLister{services: stubServices()} with zero jobs. This is fine — tests that need jobs override the client — but it means the existing filter tests (TestSnapshotCloudRunFilter_Include, etc.) only exercise filtering on services. Since jobs use the identical ShouldInclude call, this is not a gap, just worth keeping in mind if the filter logic ever diverges by kind.

  2. Success log says "artifacts" not "services and jobs" (snapshotCloudRun.go:180): [%d] artifacts were reported is accurate and kind-neutral, which is an improvement over the old "revisions" wording. No action needed.

  3. The listJobs iterator in gcpAPI (cloudrun.go:256-270) follows the exact same iterator.Done pattern as listServices — consistent and correct.

Verdict: This PR is ready to merge. Clean implementation, thorough tests, good help text. Nice work.

Comment thread cmd/kosli/snapshotCloudRun.go
Comment thread internal/cloudrun/cloudrun.go
@ToreMerkely ToreMerkely marked this pull request as draft May 8, 2026 07:07
auto-merge was automatically disabled May 8, 2026 07:07

Pull request was converted to draft

ToreMerkely and others added 2 commits May 8, 2026 09:11
…lude-regex (Slice 4, #4986)

The cloud-run filter now applies uniformly to Cloud Run service names
and Cloud Run Job names (since Slice 3). The old "--services" naming
is misleading once jobs are in the mix, so flip to a kind-neutral
"--include" / "--include-regex" pair. The exclude flags keep their
names but their descriptions are updated to reflect the broader scope.

  - --services      → --include
  - --services-regex → --include-regex
  - --exclude       (description: now "service or job names")
  - --exclude-regex (description: same)

Also rename the internal `serviceFilter` field on
snapshotCloudRunOptions to `resourceFilter`, matching the field's now-
broader scope. PreRunE mutex pairs and error messages follow the new
flag names.

The cloud-run command and the server's cloud-run env type are still
hidden / pre-release, so this is a free rename — no users depend on
the old flag names yet.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The short and long descriptions for \`kosli snapshot cloud-run\` only
mentioned services. Update them to:

  - Explicitly cover both kinds at the top level ("services and jobs").
  - Spell out the per-kind artifact mapping: services emit one artifact
    per traffic revision, jobs emit one artifact per Job (since Jobs
    don't have a revision/traffic-split model).
  - Note that idle Jobs (no running Execution) are included — the
    headline value-add of #4986.
  - Document that the --include / --exclude filters apply uniformly to
    both service and job names.

The "hidden, in-development" disclaimer stays.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ToreMerkely
Copy link
Copy Markdown
Contributor Author

Addressed the three follow-up items from the review summary:

  • A. Help text doesn't mention jobs — fixed in b4acef8 (Slice 5). snapshotCloudRunShortDesc and snapshotCloudRunLongDesc now cover both services and jobs, including the per-kind artifact mapping and the "idle Jobs are included" headline of #4986.
  • B. Flag descriptions only reference services — fixed in 40c3ae2 (Slice 4). Renamed --services--include and --services-regex--include-regex since the filters now apply uniformly to both kinds. All four flag descriptions updated to "Cloud Run service or job names".
  • C. Nit: rename serviceFilterresourceFilter — also done in 40c3ae2.

The cloud-run command and the server's cloud-run env type are still hidden / pre-release, so the flag rename is a free change with no users on the old names.

Comment thread cmd/kosli/snapshotCloudRun_test.go
Comment thread internal/cloudrun/cloudrun.go
@ToreMerkely ToreMerkely marked this pull request as ready for review May 8, 2026 07:25
@ToreMerkely ToreMerkely enabled auto-merge (squash) May 8, 2026 07:25
@ToreMerkely ToreMerkely disabled auto-merge May 8, 2026 07:28
… 6, #4986)

\`kosli snapshot cloud-run\` was the only \`snapshot\` subcommand without
an \`Example\` block. Bring it to parity with ECS / Lambda / S3 / K8S /
Azure Apps / Docker / Path / Server, and flesh out the Long description.

  - New snapshotCloudRunExample with three idiomatic invocations: full
    snapshot, --include subset, and --exclude (with kosli-reporter as
    the canonical "skip the Job that runs this command" case).
  - Long description gains: a paragraph on GCP Application Default
    Credentials and the roles/run.viewer requirement, and a paragraph
    on filter usage (include vs exclude mutex, case-sensitivity, both
    kinds covered) — same style ECS uses.
  - \`Example: snapshotCloudRunExample\` wired into the cobra command.

Help output and the Mintlify docs generator both pick this up. Caret
quoting (^...^) is the existing convention — the docgen replaces it
with backticks; --help shows literal carets, same as every other
command.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ToreMerkely ToreMerkely merged commit 13da1ed into main May 8, 2026
19 checks passed
@ToreMerkely ToreMerkely deleted the 4986-cloud-run-job-reporting-2 branch May 8, 2026 07:48
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.

2 participants