-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: cli option to enable the new action cache #1954
feat: cli option to enable the new action cache #1954
Conversation
// Mkdir | ||
buf := &bytes.Buffer{} | ||
tw := tar.NewWriter(buf) | ||
_ = tw.WriteHeader(&tar.Header{ | ||
Name: destPath, | ||
Mode: 777, | ||
Typeflag: tar.TypeDir, | ||
}) | ||
tw.Close() | ||
err := cr.cli.CopyToContainer(ctx, cr.id, "/", buf, types.CopyToContainerOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("failed to mkdir to copy content to container: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second CopyToContainer would fail with destPath doesn't exists.
Not using mkdir, works in containers without that tool and use plain docker api.
// If this fails, then folders have wrong permissions on non root container | ||
if cr.UID != 0 || cr.GID != 0 { | ||
_ = cr.Exec([]string{"chown", "-R", fmt.Sprintf("%d:%d", cr.UID, cr.GID), destPath}, nil, "0", "")(ctx) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before we need to fix folder ownership
pkg/runner/action.go
Outdated
rc := step.getRunContext() | ||
stepModel := step.getStepModel() | ||
|
||
if stepModel.Type() != model.StepTypeUsesActionRemote { | ||
if stepModel.Type() != model.StepTypeUsesActionRemote || rc.ActionCache == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added to avoid a panic in a Test
pkg/runner/run_context.go
Outdated
@@ -47,6 +47,7 @@ type RunContext struct { | |||
Masks []string | |||
cleanUpJobContainer common.Executor | |||
caller *caller // job calling this RunContext (reusable workflows) | |||
ActionCache ActionCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to runner.Config?
pkg/runner/step_action_remote.go
Outdated
sar.cacheDir = fmt.Sprintf("%s/%s", sar.remoteAction.Org, sar.remoteAction.Repo) | ||
sar.resolvedSha, err = cache.Fetch(ctx, sar.cacheDir, sar.remoteAction.URL+"/"+sar.cacheDir, sar.remoteAction.Ref, github.Token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolve the action and cache access variables
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1954 +/- ##
==========================================
- Coverage 61.22% 60.72% -0.51%
==========================================
Files 46 53 +7
Lines 7141 8959 +1818
==========================================
+ Hits 4372 5440 +1068
- Misses 2462 3081 +619
- Partials 307 438 +131 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a feature preview, a cli flag controls if this feature is on.
Next steps are cli flags for local repository replacements instead of cloning the action.
E.g. developing an actions pre step in act.
@@ -612,6 +613,11 @@ func newRunCommand(ctx context.Context, input *Input) func(*cobra.Command, []str | |||
ReplaceGheActionTokenWithGithubCom: input.replaceGheActionTokenWithGithubCom, | |||
Matrix: matrixes, | |||
} | |||
if input.useNewActionCache { | |||
config.ActionCache = &runner.GoGitActionCache{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabled behind feature cli flag: --use-new-action-cache
, old code has been restored.
See ChristopherHX#34 for what is possible with substitution. The filecollector should be moved out of the container package and made public. |
Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de>
@ChristopherHX this pull request has failed checks 🛠 |
Just added support for remote reusable workflows, should be possible to be redirected them to a local folder via https://github.com/nektos/act/tree/local-repository-action-cache
w.yml on: push
jobs:
caller:
uses: Christopher/re-complex/.github/workflows/callable.yml@main
with:
os: ubuntu-latest
secrets:
API_TOKEN: ${{ tojson(github.sha) }} .github/workflows/callable.yml on:
workflow_call:
inputs:
os:
required: true
type: string
secrets:
API_TOKEN:
required: true
jobs:
test-scripts:
runs-on: ${{ inputs.os }}
steps:
- run: echo ${{ secrets.API_TOKEN }} |
@ChristopherHX this pull request is now in conflict 😩 |
Seems like we decided to no longer try to fix concurrent action / reusable workflow updates. Am I allowed to continue here, or what are your concerns? |
func symlinkJoin(filename, sym, parent string) (string, error) { | ||
dir := path.Dir(filename) | ||
dest := path.Join(dir, sym) | ||
prefix := path.Clean(parent) + "/" | ||
if strings.HasPrefix(dest, prefix) || prefix == "./" { | ||
return dest, nil | ||
} | ||
return "", fmt.Errorf("symlink tries to access file '%s' outside of '%s'", strings.ReplaceAll(dest, "'", "''"), strings.ReplaceAll(parent, "'", "''")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget that \
are technically allowed here, path
wouldn't handle them...
Don't think it is a new problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally 👍
* Enable the new action cache * fix * fix: CopyTarStream (Docker) * suppress panic in test * add a cli option for opt in * fixups * add package * fix * rc.Config nil in test??? * add feature flag * patch * Fix respect --action-cache-path Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de> * add remote reusable workflow to ActionCache * fixup --------- Co-authored-by: Björn Brauer <zaubernerd@zaubernerd.de> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
This is my initial code to switch the action cache over to my new implementation
Until all tests are migrated this will be kept as draft to avoid mergify spam.
I would be careful to test this change, to detect known issues of my previous PR's and this change.
Checklist
Do you prefer to add a Feature Toggle for this change? Then we can merge this while iron out remaining bugs later and make it possible to make use of this change for people affected by the current problems.