Skip to content

Use always absolute output path#344

Merged
canercidam merged 3 commits intomainfrom
ferranbt-fix-relative-output-host-path
Feb 16, 2026
Merged

Use always absolute output path#344
canercidam merged 3 commits intomainfrom
ferranbt-fix-relative-output-host-path

Conversation

@ferranbt
Copy link
Contributor

Fixes #343

@ferranbt ferranbt requested a review from canercidam as a code owner January 28, 2026 08:16
@claude
Copy link
Contributor

claude bot commented Jan 28, 2026

Claude finished @ferranbt's task in 1m 16s —— View job


Review Complete


Summary: This PR fixes the output path handling to always use absolute paths when a custom output directory is specified. The changes look correct.

Changes reviewed:

  1. playground/artifacts.go:525-532 - Adds conversion of relative paths to absolute paths in NewOutput(). This ensures that when a relative path like ./output is passed, it gets converted to an absolute path before being stored.

  2. playground/components_test.go:203-204 - Updates tests to use NewOutput() instead of directly constructing the output struct. This is the correct approach since it ensures tests exercise the same code path as production.

Observations:

  • The AbsoluteDstPath() method at line 559 now becomes somewhat redundant since dst is already guaranteed to be absolute after NewOutput(). It will effectively just return o.dst. However, keeping it provides backward compatibility if there are any code paths that construct output directly without going through NewOutput().

  • The test change removes the custom homeDir assignment. Previously the test set homeDir: filepath.Join(e2eTestDir, "artifacts"), but now it uses the global home directory from GetHomeDir(). This should be fine for test purposes but is a subtle behavioral change.

No blocking issues found. LGTM.

@claude
Copy link
Contributor

claude bot commented Jan 28, 2026

Claude finished @ferranbt's task in 59s —— View job


PR Review Complete


Summary: This PR ensures that when a custom output directory is specified (e.g., ./output), it gets converted to an absolute path early in NewOutput(), rather than at usage time in toDockerComposeService().

Changes:

  1. playground/artifacts.go:525-532 - Adds conversion of relative paths to absolute paths in NewOutput() when a non-empty dst is provided.

  2. playground/artifacts.go - Removes the AbsoluteDstPath() method since dst is now guaranteed to be absolute after NewOutput().

  3. playground/local_runner.go:500 - Simplifies toDockerComposeService() to use d.out.Dst() directly instead of the now-removed AbsoluteDstPath().

  4. playground/components_test.go:203-204 - Updates tests to use NewOutput() instead of directly constructing the output struct. This ensures tests exercise the same code path as production.

No issues found. The changes are clean and the approach is sound - converting to absolute paths at construction time is better than doing it at each usage site.

@claude
Copy link
Contributor

claude bot commented Feb 16, 2026

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


Review Complete

  • Review the PR diff and changed files
  • Check for bugs, edge cases, and code quality issues
  • Post review feedback

No issues found. The changes are straightforward and correct:

  1. playground/artifacts.goNewOutput() now calls filepath.Abs(dst) unconditionally (for both default and user-provided paths), ensuring dst is always absolute from construction time. The removed AbsoluteDstPath() method is no longer referenced anywhere.

  2. playground/local_runner.gotoDockerComposeService() simplified to use d.out.Dst() directly since the path is already absolute.

  3. playground/components_test.go — Tests updated to use NewOutput() instead of constructing the struct directly, which is the right approach.

Pre-existing note (not introduced by this PR): The Exists method at playground/artifacts.go:559 ignores the path parameter — it only checks o.dst regardless of what path is passed. This is unrelated to this PR but worth fixing separately.

@canercidam canercidam merged commit 47fd55f into main Feb 16, 2026
14 checks passed
@canercidam canercidam deleted the ferranbt-fix-relative-output-host-path branch February 16, 2026 13:36
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.

Playground fails to run host component with a custom output folder

3 participants