Skip to content

Add scion build command + Dockerfile hash fix#244

Open
ptone wants to merge 3 commits into
mainfrom
scion/harness-local-build
Open

Add scion build command + Dockerfile hash fix#244
ptone wants to merge 3 commits into
mainfrom
scion/harness-local-build

Conversation

@ptone

@ptone ptone commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • New scion build CLI command (cmd/build.go) — builds container images from harness-config Dockerfiles with support for --tag, --base-image, --push, --platform, and --dry-run flags. After build, updates the harness-config's config.yaml image field.
  • Dockerfile hash fix — removes Dockerfile from skipBasenames in ComputeHarnessConfigRevision() so Dockerfile changes are reflected in content hashing and trigger re-sync to the Hub.
  • Shared DetectContainerRuntime() — extracts the container runtime detection utility from pkg/hub/maintenance_executors.go to pkg/runtime/container.go for reuse by both the build command and the Hub executor.

Files changed

File Change
cmd/build.go New — scion build command
pkg/runtime/container.go New — shared DetectContainerRuntime()
pkg/config/harness_config.go Remove Dockerfile from hash skip list
pkg/config/harness_config_test.go Update test to verify Dockerfile changes affect revision
pkg/hub/maintenance_executors.go Use shared DetectContainerRuntime(), remove local copy

Test plan

  • go build ./... passes
  • go vet clean on all changed packages
  • TestComputeHarnessConfigRevision_SkipsNonRuntimeFiles updated and passing
  • Manual: scion build <name> with a harness-config containing a Dockerfile
  • Manual: verify build not visible in SCION_CLI_MODE=agent scion --help
  • Manual: verify build visible in SCION_CLI_MODE=assistant scion --help

Introduces a top-level `scion build <harness-config-name>` CLI command
that builds a container image from a Dockerfile bundled in a harness-config
directory. Supports --tag, --base-image, --push, --platform, and --dry-run
flags. After a successful build, updates the harness-config's config.yaml
image field to reference the built image.

Also fixes Dockerfile content hashing: Dockerfiles were previously excluded
from ComputeHarnessConfigRevision, so changes to them did not trigger
re-sync to the Hub. Removes Dockerfile from the skipBasenames exclusion list.

Extracts detectContainerRuntime() from pkg/hub/maintenance_executors.go
into a shared pkg/runtime/container.go so both the new build command and
the Hub executor can use it.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a new build command to build container images from a harness-config Dockerfile, extracts container runtime detection into a shared package, and ensures that Dockerfile changes are included when computing harness-config revisions. Feedback on the changes highlights a critical issue in cmd/build.go where updating config.yaml via unmarshaling and marshaling into a struct can cause data and formatting loss. It is recommended to use yaml.Node for in-place modification and to perform an atomic file write to prevent corruption.

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.

Comment thread cmd/build.go Outdated
Comment on lines +134 to +145
var configEntry config.HarnessConfigEntry
if err := yaml.Unmarshal(configData, &configEntry); err != nil {
return fmt.Errorf("failed to parse config.yaml: %w", err)
}
configEntry.Image = outputImage
updatedData, err := yaml.Marshal(&configEntry)
if err != nil {
return fmt.Errorf("failed to marshal updated config.yaml: %w", err)
}
if err := os.WriteFile(configPath, updatedData, 0644); err != nil {
return fmt.Errorf("failed to write updated config.yaml: %w", err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Updating config.yaml by unmarshaling into a struct (config.HarnessConfigEntry) and marshaling it back has two major issues:

  1. Data Loss: Any fields in config.yaml that are not mapped in the HarnessConfigEntry struct (e.g., newer fields or custom extensions) will be silently discarded.
  2. Comment/Formatting Loss: All comments, custom formatting, and key ordering in the YAML file will be stripped, which degrades the user experience for manually managed configuration files.

Additionally, writing directly to configPath using os.WriteFile is not atomic. If the write fails (e.g., due to disk full or power loss), the configuration file can be corrupted or truncated.

Recommendation:
Use yaml.Node from gopkg.in/yaml.v3 to parse the document as an AST, modify the image field in-place, and write the updated YAML atomically using a temporary file and os.Rename.

		var node yaml.Node
		if err := yaml.Unmarshal(configData, &node); err != nil {
			return fmt.Errorf("failed to parse config.yaml: %w", err)
		}

		if len(node.Content) == 0 {
			node.Kind = yaml.DocumentNode
			node.Content = []*yaml.Node{
				{Kind: yaml.MappingNode, Tag: "!!map"},
			}
		}

		updated := false
		if node.Content[0].Kind == yaml.MappingNode {
			mapping := node.Content[0]
			for i := 0; i < len(mapping.Content); i += 2 {
				if mapping.Content[i].Value == "image" {
					mapping.Content[i+1].Value = outputImage
					mapping.Content[i+1].Tag = "!!str"
					updated = true
					break
				}
			}
			if !updated {
				mapping.Content = append(mapping.Content,
					&yaml.Node{Kind: yaml.ScalarNode, Value: "image", Tag: "!!str"},
					&yaml.Node{Kind: yaml.ScalarNode, Value: outputImage, Tag: "!!str"},
				)
			}
		}

		updatedData, err := yaml.Marshal(&node)
		if err != nil {
			return fmt.Errorf("failed to marshal updated config.yaml: %w", err)
		}

		tmpPath := configPath + ".tmp"
		if err := os.WriteFile(tmpPath, updatedData, 0644); err != nil {
			return fmt.Errorf("failed to write temporary config.yaml: %w", err)
		}
		if err := os.Rename(tmpPath, configPath); err != nil {
			return fmt.Errorf("failed to replace config.yaml atomically: %w", err)
		}

Scion Agent (harness-local-build-p1) added 2 commits June 11, 2026 18:16
…tings

H1: Replace destructive yaml.Unmarshal/Marshal round-trip with targeted
yaml.Node edit for config.yaml image update. Preserves comments, field
order, and unknown fields.

M1: Handle all os.Stat errors on Dockerfile, not just IsNotExist.

M2: Load settings once instead of twice when both --base-image is unset
and --push is set.

L3: Remove extra blank line in maintenance_executors.go.
Add empty-path check after FindHarnessConfigDir to prevent synthetic
harness-configs (e.g. 'generic') from resolving Dockerfile against CWD.

Verify yaml.MappingNode kind before manipulating doc.Content[0] to
handle malformed config.yaml gracefully.
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.

1 participant