Skip to content

refactor(orchestrator): reuse parsed metadata for copy-build SQL output - #3308

Merged
arkamar merged 1 commit into
mainfrom
refactor/copy-build-sql-from-metadata
Jul 20, 2026
Merged

arkamar merged 1 commit into
mainfrom
refactor/copy-build-sql-from-metadata

Conversation

@arkamar

@arkamar arkamar commented Jul 20, 2026

Copy link
Copy Markdown
Member

The -team SQL path re-read metadata.json from the destination with an ad-hoc struct just to extract kernel and firecracker versions. Reuse the metadata already parsed from the source at the start of the copy, introduced in 62add04 ("fix(orchestrator): make copy-build handle filesystem-only snapshots (#3299)").

Deprecated (V1) metadata is stripped to a bare version on deserialize, so its kernel/firecracker versions are no longer available to the SQL path; fail loudly for such builds instead of emitting an env_builds row with empty version strings.

The -team SQL path re-read metadata.json from the destination with an
ad-hoc struct just to extract kernel and firecracker versions. Reuse the
metadata already parsed from the source at the start of the copy,
introduced in 62add04 ("fix(orchestrator): make copy-build handle
filesystem-only snapshots (#3299)").

Deprecated (V1) metadata is stripped to a bare version on deserialize,
so its kernel/firecracker versions are no longer available to the SQL
path; fail loudly for such builds instead of emitting an env_builds row
with empty version strings.
@cursor

cursor Bot commented Jul 20, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Scope is the copy-build CLI and optional SQL output; behavior change is failing on legacy metadata rather than writing incomplete DB rows.

Overview
With -team, SQL generation no longer reads metadata.json from the copy destination to get kernel and Firecracker versions; it uses the metadata already loaded from the source when the copy starts. If those version fields are empty (e.g. deprecated V1 metadata), the tool fatals instead of inserting env_builds rows with blank version strings.

Reviewed by Cursor Bugbot for commit dee410a. Bugbot is set up for automated code reviews on this repo. Configure here.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the logic for reading and decoding metadata.json from GCS or local storage in packages/orchestrator/cmd/copy-build/main.go. However, this change leaves the meta variable undefined, which will result in a compilation error. There are no review comments to evaluate, and thus we have no feedback to provide on the review.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jul 20, 2026 •

Copy link
Copy Markdown

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
3425 2 3423 9
View the top 2 failed test(s) by shortest run time
github.com/e2b-dev/infra/tests/integration/internal/tests/api/templates::TestAssignmentOrderingAfterTagReassignment
Stack Traces | 158s run time
=== RUN   TestAssignmentOrderingAfterTagReassignment
=== PAUSE TestAssignmentOrderingAfterTagReassignment
=== CONT  TestAssignmentOrderingAfterTagReassignment
    template_tags_test.go:589: Build failed: {<nil> An internal error occurred. Please try again or contact support with the build ID. <nil>}
--- FAIL: TestAssignmentOrderingAfterTagReassignment (157.60s)
github.com/e2b-dev/infra/tests/integration/internal/tests/api/templates::TestAssignmentOrderingLatestWins
Stack Traces | 165s run time
=== RUN   TestAssignmentOrderingLatestWins
=== PAUSE TestAssignmentOrderingLatestWins
=== CONT  TestAssignmentOrderingLatestWins
    template_tags_test.go:522: Build failed: {<nil> An internal error occurred. Please try again or contact support with the build ID. <nil>}
--- FAIL: TestAssignmentOrderingLatestWins (164.89s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — straightforward refactor that removes a redundant metadata re-read and closes a silent-failure gap.

Extended reasoning...

Overview

Single-file change to packages/orchestrator/cmd/copy-build/main.go, an operator-run CLI tool (not a long-running service). It removes the ad-hoc second parse of metadata.json from the destination (used only for the -team SQL-generation path) and instead reuses the metadata.Template already parsed from the source at the top of main() (added in #3299). It also adds an explicit log.Fatalf when kernel/firecracker version fields are empty, instead of silently emitting an env_builds row with blank version strings.

Security risks

None. This tool is invoked manually by operators/CI to copy build artifacts and optionally emit SQL for manual DB seeding; there's no new external input handling, and the SQL is still printed to stdout for a human to run, unchanged from before.

Level of scrutiny

Low-to-moderate is appropriate: it's an internal ops utility, the diff is small and mechanical, and I confirmed the semantics against packages/orchestrator/pkg/template/metadata/template_metadata.go — deserialize() does strip V1/deprecated metadata down to a bare Template{Version: DeprecatedVersion}, so the new empty-version check correctly catches that case and fails loudly rather than emitting bad SQL, matching the PR description. Reusing the source-parsed meta instead of re-reading from the destination is safe here because the destination copy is verified to succeed (via the earlier errgroup.Wait()) before the -team block runs, so source and destination metadata are equivalent by construction.

Other factors

Verified the file still builds (go build ./cmd/copy-build/...) after the import removal. No tests exist for this CLI (consistent with the rest of the file, which was already untested before this PR), and the bug-hunting pass found no issues.

@arkamar
arkamar merged commit e0f0528 into main Jul 20, 2026
43 checks passed
@arkamar
arkamar deleted the refactor/copy-build-sql-from-metadata branch July 20, 2026 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants