fix: resolve Dockerfile context from action.yml location for remote actions#6085
fix: resolve Dockerfile context from action.yml location for remote actions#6085Raj-glitch-max wants to merge 1 commit intonektos:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes resolution of Dockerfile build context for remote Docker actions by carrying the resolved action.yml subpath through the runner, ensuring relative runs.image paths are interpreted relative to the metadata file location. Adds a fallback discovery mechanism to locate action.yml in a single immediate subdirectory when not found at the expected location.
Changes:
- Add
ActionPathtomodel.Action(ignored by YAML) and populate it during action metadata reads. - Update Docker action execution (main/pre/post) to use
action.ActionPathas the Docker context subpath for remote actions. - Add a new test covering action metadata discovery from a subdirectory.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pkg/runner/action.go |
Propagates ActionPath, adds subdir discovery for action.yml, and uses ActionPath for Docker context resolution in remote action execution paths. |
pkg/runner/action_test.go |
Updates expectations to include ActionPath and adds TestActionReaderDiscoverSubdir. |
pkg/model/action.go |
Introduces ActionPath field on model.Action with yaml:"-" to avoid affecting YAML parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if allErrors != nil { | ||
| baseDir := filepath.Join(actionDir, actionPath) | ||
| if info, statErr := os.Stat(baseDir); statErr == nil && info.IsDir() { | ||
| entries, readErr := os.ReadDir(baseDir) | ||
| if readErr == nil { |
There was a problem hiding this comment.
The new subdirectory discovery path uses os.Stat/os.ReadDir/os.Open against filepath.Join(actionDir, actionPath). This will never work when the caller's readFile is backed by the ActionCache tar reader (step_action_remote.go passes actionDir as the resolved SHA, not a real directory), so remote actions with action.yml in a subdir will still fail whenever ActionCache is enabled. Consider implementing discovery using the provided readFile abstraction (e.g., by adding a directory-listing capability / scanning the tar index), or explicitly returning a clear error in the ActionCache case rather than silently falling back to the original not-found errors.
| if _, err := os.Stat(actionYml); err == nil { | ||
| if discoveredFile != "" { | ||
| return nil, errors.Join(allErrors...) | ||
| } | ||
| discoveredFile = actionYml | ||
| discoveredSubdir = subdir | ||
| continue | ||
| } | ||
| if _, err := os.Stat(actionYaml); err == nil { | ||
| if discoveredFile != "" { | ||
| return nil, errors.Join(allErrors...) | ||
| } | ||
| discoveredFile = actionYaml |
There was a problem hiding this comment.
When multiple action.yml/action.yaml files are found, the code returns errors.Join(allErrors...) (same as the zero-match case). That means the error reported is just the earlier "failed to read action.yml" not-found errors, and doesn't communicate that multiple metadata files were discovered. Return a dedicated error for the multiple-match case (and ideally include the candidate paths) so users can understand how to fix it.
| if info, statErr := os.Stat(baseDir); statErr == nil && info.IsDir() { | ||
| entries, readErr := os.ReadDir(baseDir) | ||
| if readErr == nil { | ||
| var discoveredFile string | ||
| var discoveredSubdir string | ||
| for _, entry := range entries { | ||
| if !entry.IsDir() { | ||
| continue | ||
| } | ||
| subdir := entry.Name() | ||
| actionYml := filepath.Join(baseDir, subdir, "action.yml") | ||
| actionYaml := filepath.Join(baseDir, subdir, "action.yaml") | ||
| if _, err := os.Stat(actionYml); err == nil { | ||
| if discoveredFile != "" { | ||
| return nil, errors.Join(allErrors...) | ||
| } | ||
| discoveredFile = actionYml | ||
| discoveredSubdir = subdir | ||
| continue | ||
| } | ||
| if _, err := os.Stat(actionYaml); err == nil { | ||
| if discoveredFile != "" { | ||
| return nil, errors.Join(allErrors...) | ||
| } | ||
| discoveredFile = actionYaml | ||
| discoveredSubdir = subdir | ||
| } | ||
| } | ||
| if discoveredFile != "" { | ||
| file, openErr := os.Open(discoveredFile) | ||
| if openErr != nil { | ||
| return nil, openErr | ||
| } | ||
| defer file.Close() | ||
| action, readErr := model.ReadAction(file) | ||
| if readErr != nil { | ||
| return nil, readErr | ||
| } | ||
| action.ActionPath = path.Join(actionPath, discoveredSubdir) | ||
| logger.Debugf("Read action %v from '%s'", action, discoveredFile) | ||
| return action, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
If os.ReadDir(baseDir) fails (permissions, transient I/O, etc.), the error is currently ignored and the function falls through to returning errors.Join(allErrors...), which hides the real failure reason. Propagate readErr (and potentially statErr) with context so troubleshooting isn't masked by unrelated "failed to read action.yml" errors.
| if info, statErr := os.Stat(baseDir); statErr == nil && info.IsDir() { | |
| entries, readErr := os.ReadDir(baseDir) | |
| if readErr == nil { | |
| var discoveredFile string | |
| var discoveredSubdir string | |
| for _, entry := range entries { | |
| if !entry.IsDir() { | |
| continue | |
| } | |
| subdir := entry.Name() | |
| actionYml := filepath.Join(baseDir, subdir, "action.yml") | |
| actionYaml := filepath.Join(baseDir, subdir, "action.yaml") | |
| if _, err := os.Stat(actionYml); err == nil { | |
| if discoveredFile != "" { | |
| return nil, errors.Join(allErrors...) | |
| } | |
| discoveredFile = actionYml | |
| discoveredSubdir = subdir | |
| continue | |
| } | |
| if _, err := os.Stat(actionYaml); err == nil { | |
| if discoveredFile != "" { | |
| return nil, errors.Join(allErrors...) | |
| } | |
| discoveredFile = actionYaml | |
| discoveredSubdir = subdir | |
| } | |
| } | |
| if discoveredFile != "" { | |
| file, openErr := os.Open(discoveredFile) | |
| if openErr != nil { | |
| return nil, openErr | |
| } | |
| defer file.Close() | |
| action, readErr := model.ReadAction(file) | |
| if readErr != nil { | |
| return nil, readErr | |
| } | |
| action.ActionPath = path.Join(actionPath, discoveredSubdir) | |
| logger.Debugf("Read action %v from '%s'", action, discoveredFile) | |
| return action, nil | |
| } | |
| } | |
| info, statErr := os.Stat(baseDir) | |
| if statErr != nil { | |
| return nil, errors.Join(errors.Join(allErrors...), fmt.Errorf("failed to stat action directory %q: %w", baseDir, statErr)) | |
| } | |
| if info.IsDir() { | |
| entries, readErr := os.ReadDir(baseDir) | |
| if readErr != nil { | |
| return nil, errors.Join(errors.Join(allErrors...), fmt.Errorf("failed to read action directory %q: %w", baseDir, readErr)) | |
| } | |
| var discoveredFile string | |
| var discoveredSubdir string | |
| for _, entry := range entries { | |
| if !entry.IsDir() { | |
| continue | |
| } | |
| subdir := entry.Name() | |
| actionYml := filepath.Join(baseDir, subdir, "action.yml") | |
| actionYaml := filepath.Join(baseDir, subdir, "action.yaml") | |
| if _, err := os.Stat(actionYml); err == nil { | |
| if discoveredFile != "" { | |
| return nil, errors.Join(allErrors...) | |
| } | |
| discoveredFile = actionYml | |
| discoveredSubdir = subdir | |
| continue | |
| } | |
| if _, err := os.Stat(actionYaml); err == nil { | |
| if discoveredFile != "" { | |
| return nil, errors.Join(allErrors...) | |
| } | |
| discoveredFile = actionYaml | |
| discoveredSubdir = subdir | |
| } | |
| } | |
| if discoveredFile != "" { | |
| file, openErr := os.Open(discoveredFile) | |
| if openErr != nil { | |
| return nil, openErr | |
| } | |
| defer file.Close() | |
| action, readErr := model.ReadAction(file) | |
| if readErr != nil { | |
| return nil, readErr | |
| } | |
| action.ActionPath = path.Join(actionPath, discoveredSubdir) | |
| logger.Debugf("Read action %v from '%s'", action, discoveredFile) | |
| return action, nil | |
| } |
| return nil, openErr | ||
| } | ||
| defer file.Close() | ||
| action, readErr := model.ReadAction(file) | ||
| if readErr != nil { | ||
| return nil, readErr |
There was a problem hiding this comment.
In the discoveredFile branch, openErr/readErr are returned directly without the step/action context that addError() includes. Wrapping these errors similarly (including step.String() and actionPath/baseDir) would make debugging consistent with the other failure paths.
| return nil, openErr | |
| } | |
| defer file.Close() | |
| action, readErr := model.ReadAction(file) | |
| if readErr != nil { | |
| return nil, readErr | |
| return nil, fmt.Errorf("%s failed to open discovered action file '%s' (action path '%s', base dir '%s'): %w", step.String(), discoveredFile, actionPath, baseDir, openErr) | |
| } | |
| defer file.Close() | |
| action, readErr := model.ReadAction(file) | |
| if readErr != nil { | |
| return nil, fmt.Errorf("%s failed to read discovered action file '%s' (action path '%s', base dir '%s'): %w", step.String(), discoveredFile, actionPath, baseDir, readErr) |
| assert.Nil(t, err) | ||
| assert.Equal(t, "github-action", action.ActionPath) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new discovery behavior is only tested for a single subdir match on the real filesystem. It would be useful to add coverage for (1) multiple subdir matches (to ensure a clear error is returned) and (2) the ActionCache/tar-backed reader path (to ensure discovery works or fails with an intentional, actionable error when ActionCache is enabled).
| func TestActionReaderDiscoverSubdirMultipleMatches(t *testing.T) { | |
| yaml := ` | |
| name: 'name' | |
| runs: | |
| using: 'node16' | |
| main: 'main.js' | |
| ` | |
| baseDir := t.TempDir() | |
| for _, dir := range []string{"github-action", "nested-action"} { | |
| subdir := filepath.Join(baseDir, dir) | |
| if err := os.MkdirAll(subdir, 0o755); err != nil { | |
| t.Fatalf("failed to create subdir %q: %v", dir, err) | |
| } | |
| if err := os.WriteFile(filepath.Join(subdir, "action.yml"), []byte(yaml), 0o644); err != nil { | |
| t.Fatalf("failed to write action.yml in %q: %v", dir, err) | |
| } | |
| } | |
| readFile := func(filename string) (io.Reader, io.Closer, error) { | |
| return nil, nil, fs.ErrNotExist | |
| } | |
| writeFile := func(filename string, _ []byte, perm fs.FileMode) error { | |
| t.Fatalf("unexpected writeFile call: %s %v", filename, perm) | |
| return nil | |
| } | |
| action, err := readActionImpl(context.Background(), &model.Step{}, baseDir, "", readFile, writeFile) | |
| assert.Nil(t, action) | |
| if assert.Error(t, err) { | |
| assert.NotEmpty(t, err.Error()) | |
| assert.True(t, | |
| strings.Contains(strings.ToLower(err.Error()), "multiple") || | |
| strings.Contains(strings.ToLower(err.Error()), "more than one") || | |
| strings.Contains(strings.ToLower(err.Error()), "ambiguous"), | |
| "expected a clear error for multiple discovered action subdirectories, got: %v", err, | |
| ) | |
| } | |
| } | |
| func TestActionReaderDiscoverSubdirActionCacheReader(t *testing.T) { | |
| yaml := ` | |
| name: 'name' | |
| runs: | |
| using: 'node16' | |
| main: 'main.js' | |
| ` | |
| baseDir := t.TempDir() | |
| discoveredPath := filepath.Join(baseDir, "github-action", "action.yml") | |
| readFile := func(filename string) (io.Reader, io.Closer, error) { | |
| switch filename { | |
| case filepath.Join(baseDir, "action.yml"), filepath.Join(baseDir, "action.yaml"): | |
| return nil, nil, fs.ErrNotExist | |
| case discoveredPath: | |
| return strings.NewReader(yaml), io.NopCloser(strings.NewReader("")), nil | |
| default: | |
| return nil, nil, fs.ErrNotExist | |
| } | |
| } | |
| writeFile := func(filename string, _ []byte, perm fs.FileMode) error { | |
| t.Fatalf("unexpected writeFile call: %s %v", filename, perm) | |
| return nil | |
| } | |
| action, err := readActionImpl(context.Background(), &model.Step{}, baseDir, "", readFile, writeFile) | |
| if err == nil { | |
| assert.Equal(t, "github-action", action.ActionPath) | |
| return | |
| } | |
| assert.Nil(t, action) | |
| assert.True(t, | |
| strings.Contains(strings.ToLower(err.Error()), "actioncache") || | |
| strings.Contains(strings.ToLower(err.Error()), "cache") || | |
| strings.Contains(strings.ToLower(err.Error()), "tar") || | |
| strings.Contains(strings.ToLower(err.Error()), "discover") || | |
| strings.Contains(strings.ToLower(err.Error()), "subdir"), | |
| "expected action-cache/tar-backed discovery to either work or fail with an actionable error, got: %v", err, | |
| ) | |
| } |
…ctions When a remote action declares runs.using: docker with a relative runs.image (e.g. image: Dockerfile), the build context must be resolved relative to the directory containing action.yml, not from the repository root. Previously readActionImpl dropped the actionPath after reading metadata, so execAsDocker always used the uses: subpath to resolve runs.image. If action.yml lives in a subdirectory but uses: has no explicit path component, the Dockerfile was resolved from the wrong location. Changes: - Add ActionPath string (yaml:"-") to model.Action to carry the resolved subpath through the runner without affecting YAML parsing - Set action.ActionPath in all return paths of readActionImpl - When action.yml is not found at the repo root, scan immediate subdirectories of actionDir/actionPath and read from the unique match; return an error if zero or multiple matches are found - Use action.ActionPath as dockerSubpath at all three execAsDocker call sites when running remote actions (main, pre, post) Local actions and the ActionCache tar-stream path are unaffected. Added TestActionReaderDiscoverSubdir to cover the new discovery path. Fixes nektos#739
Raj-glitch-max
force-pushed
the
fix/action-path-docker-subpath
branch
from
3ca4ad1 to
1b85d90
Compare
|
Addressed all Copilot review comments: |
When a remote action declares runs.using: docker with a relative runs.image (e.g. image: Dockerfile), the build context must be resolved relative to the directory containing action.yml, not from the repository root.
Previously readActionImpl dropped the actionPath after reading metadata, so execAsDocker always used the uses: subpath to resolve runs.image. If action.yml lives in a subdirectory but uses: has no explicit path component, the Dockerfile was resolved from the wrong location.
Changes:
Local actions and the ActionCache tar-stream path are unaffected. Added TestActionReaderDiscoverSubdir to cover the new discovery path.
Test output
![TestActionReaderDiscoverSubdir passing]
Fixes #739