feat: add 'using: docker' action pre and post-entrypoint support #1236
viceice
aahlenst
mfenniak
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/runner!1236
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "achyrva/runner:entry-point-handling-0"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Add /pre- and /post-entrypoint handling
WIP: Add /pre- and /post-entrypoint handlingto Add /pre- and /post-entrypoint handling@achyrva Could you tell us more about this change? What problem does it solve? Are there any alternatives? Is there an existing feature request for it?
@aahlenst wrote in #1236 (comment):
Hello. Yeah, sure.
There is no existing feature request for these changes.
These changed was done for Nektos/Act some time ago. Here is the reference to the original issue https://github.com/nektos/act/issues/2363 and my PR to solve it https://github.com/nektos/act/pull/2394.
So, here, I just adopt my code to current state of Forgejo Runner
@aahlenst wrote in #1236 (comment):
It is part of the action specification and it makes it possible to do some setup and cleanup from an action.
We need at the moment to cleanup some proxmox vms after tests has been executed
@klausfyhn wrote in #1236 (comment):
Do you have a link to the specification?
@aahlenst wrote in #1236 (comment):
By specification I mean the jsonschema:
"pre-entrypoint": "non-empty-string",And
"post-entrypoint": "non-empty-string",It is also mentioned in the GitHub documentation https://docs.github.com/en/actions/reference/workflows-and-actions/metadata-syntax#runspre-entrypoint
Revisions are required to reduce the scope of unrelated change in this PR, and the tests are quite insufficient.
I'd also like to see a functional integration test in the
act/runnersystem similar to the existingRunEventtests, as all the testing is unit-test level and doesn't actually execute... well, some of it doesn't execute anything?... but in general it doesn't execute the functionality end-to-end to verify it works.@ -201,4 +200,2 @@logger.Debugf("executing remote job container: %s", containerArgs)rc.ApplyExtraPath(ctx, step.getEnv())Please don't reformat code that isn't related to the change being made. Throughout this PR, I'd appreciate it if you could cleanup the diff to just be affecting code that you're intending to change to implement the pre/post entrypoint, and not removing or adding other whitespace.
@ -350,0 +361,4 @@entrypoint, err = shellquote.Split(action.Runs.PostEntrypoint)if err != nil {return err}These blocks are quite repetitive in nature, rather than setting a variable to the appropriate entrypoint and having one block of code that performs
shellquote.Split.Fixed in
!1236 (commit
85c1fdb9b0)@ -577,0 +598,4 @@return common.NewPipelineExecutor(rc.execJobContainer(buildArgs, *step.getEnv(), "", containerActionDir),rc.execJobContainer(execArgs, *step.getEnv(), "", ""),)(ctx)It looks like you've done a significant refactor to reduce duplicated code in the
ActionRunsUsingGoblock. This should be submitted as a separate PR please, so that the changes here can be focused on the pre & post entrypoint. Fewer lines of code and fewer focuses in one PR makes it easier to review and analyze the changes, and reduces the risk of unexpected change.Yes. I did refactor because there are some code duplications in different case-blocks. So, to provide functional changes easily I did quite refactoring.
I agree that this can be divided into different PRs. Should I add it as prerequisite PR to this one or just split current one to two? I appreciate all your suggestions.
@mfenniak what is your recommendation?
Done
@ -0,0 +343,4 @@// If this compiles, the signature is correct// execAsDocker(ctx, step, actionName, basedir, subpath, localAction, entrypointType)assert.True(t, true, "Function signature validation passed")What is this test?
Removed as redundant. @klausfyhn prepared integration tests
@ -0,0 +356,4 @@// - It should call execAsDocker with "entrypoint" parameter// - For remote actions, it adjusts actionDir and actionPathassert.True(t, true, "runActionImpl correctly passes entrypoint type")What is this test?
@ -0,0 +383,4 @@// This validates that the pre and post step functions// call execAsDocker with the correct entrypoint typeassert.Equal(t, tt.entrypointType, tt.entrypointType)What is this test?
Removed as redundant. @klausfyhn prepared integration tests
Is part of this PR completed by AI-coding tools? I've never seen a human write
assert.True(t, true, .... If so, please review the Forgejo Code of Conduct and ensure that all components of the AI Agreement are being met in this PR.I have managed to do some manual testing with the changes:
v14.next.forgejo.org/klausfyhn/runner-pre-post-entrypoint-test@1d56f7969b/pre-entrypoint (L3)v14.next.forgejo.org/klausfyhn/runner-pre-post-entrypoint-test@1d56f7969b/entrypoint (L3)v14.next.forgejo.org/klausfyhn/runner-pre-post-entrypoint-test@1d56f7969b/post-entrypoint (L3)Will look into automated testing now
I did a little expirment on github and
pre-entrypointis not supported for local actions. Where could we clarify this? Would be nice if we could annotate or maybe a warning somewhere@klausfyhn wrote in #1236 (comment):
Doesn't mean we have to do the same. Do you know why GitHub does not support it? Would it make sense for Forgejo to support it? Are there any risks? What about
post-entrypoint?You could log a warning like GitHub does. Mentioning it in the docs would certainly help, too.
@mfenniak wrote in #1236 (comment):
I didn't use AI in my work.
It gives me less space to progress as developer. These tests are my first auto tests appropriate to current project. I can remove them as unnecessary.
@aahlenst wrote in #1236 (comment):
We are in a bit of "what came first, the action declaration or the pre step" problem. Since a user have to checkout the code before local action is accessible. I did the check with github.com to see if they had handled it somehow. If we want this behavior (which would be nice) I would argue it is out of scope for this PR
@achyrva wrote in #1236 (comment):
OK, thanks for answering. I was a little startled by the unusual tests and my review comments were not very helpful about them, so let me elaborate in a productive manner. Please accept my apology for the rude nature of "What is this test?" comments; a wide variety of contributors come to Forgejo projects and I'd like to be a part of making all those contributors feel welcome, and my comments absolutely did not reflect that.
Automated tests should be written to verify that the software functions as expected. They're typically structured into setting up the test environment and data, performing some action, and asserting the results of the action. I've seen this described as a the "arrange, act, assert" pattern.
Your
TestRunPrePostStepDockerSupport,TestExecAsDockerSignature, andTestRunActionImplDockerWithEntrypointTypetests don't add any value to the project because they have none of these attributes. I don't understand what your intent was with these tests, but I think it might fall into one of these two cases; either you intended to test something that the compiler is doing automatically (function signatures), or you felt there was a functional capability of this change that wasn't being tested and you left a placeholder for that to be tested in the future.As a general rule of thumb, if there are parts of your change that could be removed, and all the automated tests pass, but the functionality doesn't work -- then there's a testing gap that should be fixed. The contributors on this project can help you figure out how to write a test in an area if you describe it. If that's not the case -- eg. every change has a corresponding test that would break if the change was removed -- then test coverage is complete.
The intent of these three tests needs to be clarified and they need to be either reimplemented or removed, depending on what you feel they're here for.
@ -0,0 +14,4 @@remote:runs-on: ubuntu-lateststeps:- uses: https://v14.next.forgejo.org/klausfyhn/runner-pre-post-entrypoint-test@1e6b5aa64b061cae7dd1ac63d9538bed23a634e6@mfenniak or @aahlenst Where should I place this remote action? https://code.forgejo.org/forgejo/act-test-actions or just keep it or?
Looks like it has to go in https://code.forgejo.org/forgejo/act-test-actions because that's what's currently being used. Would be great if we could host it in the runner repository, but as that probably won't work with all test actions, there's no benefit it doing so.
In any case, wait for the opinion of somebody who's more experienced with Forgejo's infrastructure than me, for example, @mfenniak.
I've imported it into act-test-actions: https://code.forgejo.org/forgejo/act-test-actions/src/branch/main/runner-pre-post-entrypoint 👍
@mfenniak wrote in #1236 (comment):
I removed redundant unit tests because @klausfyhn prepared already solid integration tests.
@mfenniak, I would like to clarify for myself how best to deal with the remark that the refactoring part should be made separately. Should I make a separate PR as a prerequisite to the existing one? Or you have some other suggestions?
@achyrva wrote in #1236 (comment):
Options:
From my point of view, all options are possible right now because the changes now look less overwhelming than before. But generally, I prefer PRs to be as small and focused as possible.
85c1fdb9b080d789b05f80d789b05febec0612c2ebec0612c2bd6cb30214bd6cb302145d2faa9cfa5d2faa9cfaa8e5f9c012@aahlenst wrote in #1236 (comment):
I split large commit into refactoring and functional part.
@ -201,20 +201,21 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteActionlogger.Debugf("executing remote job container: %s", containerArgs)rc.ApplyExtraPath(ctx, step.getEnv())@achyrva Seems like you have reintroduced some changes to formatting, maybe your editor is playing tricks on you
I believe running
make fmtbefore committing should help.17826da950a8e5f9c012a8e5f9c012c807cfe20dc807cfe20d8d1661557d8d1661557daa4c04217f@achyrva I cleaned !1236 (commit
cd15b880c6) up for white space changes but that resulted in me being the author of the commit, that was not my intention@mfenniak and @aahlenst This PR is ready for review I believe
Thanks a lot. That was much easier to review and the tests look good. I've left some comments for your consideration.
@ -852,0 +863,4 @@`msg="some super duper awesome cleanup has happened\n" dryrun=false job="docker-with-pre-post/local " jobID=local matrix="map[]" raw_output=true stage=Post`,`msg=" ✅ Success - Post ./actions/docker-local-with-post-entrypoint" dryrun=false job="docker-with-pre-post/local " jobID=local matrix="map[]" stage=Post`,// remote`msg="docker image has prepared to do work\n" dryrun=false job=docker-with-pre-post/remote jobID=remote matrix="map[]" raw_output=true stage=Pre`,Is that the test for
pre-entrypoint? If not, where is it?In any case, we need some comments that explain where this text comes from. There should also be a mention somewhere in the test that
pre-entrypointisn't supported for actions stored in the same repository.@ -0,0 +11,4 @@- uses: https://data.forgejo.org/actions/checkout@v6- uses: ./actions/docker-local-with-post-entrypointremote:Could could split this test? One with
localand one withremote? As these tests produce large amounts of logs, smaller tests are easier to debug if something goes wrong.681c74723113a4d8c93e@aahlenst your comments should have been resolved now
@ -852,0 +859,4 @@expectedLogMessages := []string{// stage=Pre// NOTE The pre step is not possible to run for a local actions, since the declartion is first available in the main stage after checkout// TODO Create a warning if a user is triggering a local action with a pre-entrypoint@achyrva Could you have a look at this?
6a84d8aaa6d20b02acc7@achyrva I suggest you roll back the warning and make a separate PR. I have created forgejo/forgejo-actions-feature-requests#95 so we could have a discussion on how we would like to handle pre execution for local actions. This is not an issue for
dockeractions specifically@klausfyhn wrote in #1236 (comment):
reverted
43bd14fc50db8d43e5fcdb8d43e5fcb7d267a326@klausfyhn wrote in #1236 (comment):
I did it
@aahlenst, @mfenniak and @viceice Is there anything missing or what could we further improve?
Looks good to me. 👍 Thanks for making this contribution.
Add /pre- and /post-entrypoint handlingto feat: add 'using: docker' action pre and post-entrypoint support