Skip to content

feat: host_path support in recipes#401

Open
julio4 wants to merge 2 commits intomainfrom
feat/host-path-recipe
Open

feat: host_path support in recipes#401
julio4 wants to merge 2 commits intomainfrom
feat/host-path-recipe

Conversation

@julio4
Copy link
Member

@julio4 julio4 commented Mar 10, 2026

YAML recipes currently support named Docker volumes and session-local bind mounts, but not arbitrary host path bind mounts. This is needed for services that require access to host resources.

Concrete use case: log collections with docker socket.

  • Add host_path field to volume config, enabling direct host-to-container bind mounts (e.g. /var/run/docker.sock)
  • Existing volume types (name/is_local) are unchanged

@julio4 julio4 requested a review from canercidam as a code owner March 10, 2026 07:45
@claude
Copy link
Contributor

claude bot commented Mar 10, 2026

Claude finished @julio4's task in 1m 15s —— View job


PR Review: host_path support in recipes

  • Gather context and read changed files
  • Review code changes
  • Post review feedback

Overall this is a clean, well-scoped addition. A few issues to address:

1. Misplaced comment (inline comment posted on recipe_yaml.go:575-576)
The // applyFilesToService maps files to a service comment now sits above applyVolumesToService, not its intended target. Needs its own doc comment.

2. No validation of conflicting volume fields (inline comment posted on recipe_yaml.go:578)
If a user specifies both host_path and name (or host_path + is_local: true) on the same volume, host_path silently wins with no warning. Consider validating mutual exclusivity or documenting the precedence.

3. Missing validation for volume host paths
validateHostPaths in cmd_validate.go checks svc.HostPath (service binary) but not the new VolumeMapped.HostPath. A volume pointing to a nonexistent host path (e.g. typo in /var/run/docker.sock) will fail at Docker runtime with no pre-flight warning. Consider extending validation:

for containerPath, vol := range svc.VolumesMapped {
    if vol.HostPath != "" {
        if _, err := os.Stat(vol.HostPath); os.IsNotExist(err) {
            result.AddWarning("service '%s' volume '%s' host_path does not exist: %s", svc.Name, containerPath, vol.HostPath)
        }
    }
}

4. Minor: test coverage
The test covers the happy path but doesn't test the conflict case (both host_path and name set) or what happens when host_path is a relative path (unlike service-level HostPath which gets resolved via filepath.Abs, volume HostPath is passed through as-is — is this intentional?).

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@claude
Copy link
Contributor

claude bot commented Mar 10, 2026

Claude finished @julio4's task in 1m 38s —— View job


PR Review: host_path support in recipes (re-review after update)

  • Read changed files and diff
  • Check if previous review feedback was addressed
  • Post review feedback

The misplaced comment issue from the first review is now fixed — applyVolumesToService has its own doc comment and applyFilesToService is clean.

Remaining issues (still unaddressed):

1. validateHostPaths doesn't check volume host paths
cmd_validate.go:223-231 only validates svc.HostPath (service-level binary path) but not the new VolumeMapped.HostPath. A typo in a volume host path (e.g. /var/run/dockr.sock) will only fail at Docker runtime with no pre-flight warning. Consider extending:

for containerPath, vol := range svc.VolumesMapped {
    if vol.HostPath != "" {
        if _, err := os.Stat(vol.HostPath); os.IsNotExist(err) {
            result.AddWarning("service '%s' volume '%s' host_path does not exist: %s", svc.Name, containerPath, vol.HostPath)
        }
    }
}

Fix this →

2. No validation of conflicting volume fields (inline comment posted on recipe_yaml.go:578)
If a user specifies both host_path and name (or host_path + is_local: true), host_path silently wins. Consider validating mutual exclusivity or documenting the precedence.

3. Relative paths not resolved for volume host_path (inline comment posted on recipe_yaml.go:578)
Service-level HostPath gets resolved via filepath.Abs elsewhere, but volume HostPath is passed through as-is. A relative path like ./data may resolve unexpectedly at Docker runtime.

// applyVolumesToService maps volumes to a service
func applyVolumesToService(svc *Service, volumes map[string]*YAMLVolumeMappedConfig) {
for containerPath, volumeMapping := range volumes {
if volumeMapping.HostPath != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

No validation when a user sets conflicting fields (e.g. both host_path and name, or host_path + is_local: true). host_path silently wins. Consider validating mutual exclusivity here or documenting the precedence in the YAMLVolumeMappedConfig struct.

Also, unlike service-level HostPath which gets resolved via filepath.Abs, volume HostPath is passed through as-is. A relative path like ./data may resolve unexpectedly. Consider normalizing:

if volumeMapping.HostPath != "" {
    hostPath, err := filepath.Abs(volumeMapping.HostPath)
    // handle err
    svc.WithHostVolume(hostPath, containerPath)
}

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