fix: clone actions with Git instead of go-git #1255
No reviewers
limiting-factor
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!1255
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "aahlenst/runner:clone-with-git"
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?
Enables cloning of actions hosted on Git repositories that use SHA-256.
I hope I tested all scenarios including moving branches, tags, and force pushes. I couldn't test actions in private repositories because I don't know how to make it work. In any case, I would be great if reviewers would try it locally, too.
I didn't touch the locks (see #1162 for background) because I'm unsure whether it's safe to remove them.
There is no centralized debug logging of generated
gitcommands ingit()because I'm unable to obtain a context in every case, the biggest obstacle beingClose()ofgitWorktree.I found three more usages of go-git across the code base. All seem to do some kind of matching involving
.gitignore. I don't know whether it makes sense to replace them or if it's even possible. If anyone knows, let me know.@ -239,2 +174,2 @@_ = os.RemoveAll(repoDir)}func cloneIfRequired(input CloneInput, logger log.FieldLogger, repoDir string) error {if url, err := getRemoteURL(repoDir, "origin"); err == nil {the case where err != nil needs to be handled by returning the error
looking at getRemoteURL, it assumes that all errors are recoverable in the sense that they can be ignored and the function can proceed to clone the repository
in the case of an error case I think
There, the role of
getRemoteURL()is to check whether we can avoid cloning, not to ensure that cloning will succeed. That's why the code only looks at the positive case. There are many reasons why a clone could fail andgit cloneis best equipped to handle those.I'll add a comment in any case.
I don't follow.
@ -435,0 +391,4 @@workingDirectory string}func git(options *gitOptions, args ...string) (string, error) {you could borrow from https://code.forgejo.org/f3/gof3/src/branch/main/util/exec.go to run the command with a context that will bind the git command to the context and stop it when the context is cancelled
See #1255 (comment) for why there's no context, logging, or cancellation, at least not yet.
func cloneIfRequired(was called with a context that was used when callinggit.PlainCloneContextand I assume (did not check) that it would allow it to be cancelled or timeout at least in some cases that are meaningful. This is why I suggested to at least support context in the same cases in the proposed re-implementation.If you found that in the previous implementation a clone would hang forever in all cases instead of timing out, my remark is not useful.
Let me rephrase my problem:
Close()callsgit. It did so before the re-implementation. Due to wayClose()is defined, it cannot receive a context. So I either have to implementgit()forClose()without a context, add context toClose(), or leave it as it is.I have no idea which of these options is the right one.
Here is what intuitively makes sense to me: since
func (t *gitWorktree) Close() erroris short lived and does not currently need a context, usingcontext.Background()for the newgitfunction will not change its behavior. Adding a context toClose()may make sense but it seems to be out of scope for what this pull request is trying to achieve. In order to allow for a git clone to timeout or be cancelled (which is needed in general because it can hang - but again I did not check if that actually works now) the newgitfunction should run git with a context.I hope that helps. But I don't have a strong opinion because I did not thoroughly investigate the current behavior with go-git during a git clone. If it cannot timeout for some reason despite being provided with a context, then trying to add that functionality is out of scope. If it does timeout, a failure to do so in the git based re-implementation would be a regression.
I read up on
io.Closer()and there seems to be no special behaviour associated with it. And because the blast radius of adding a context is rather small, I went ahead and added a context everywhere. See8bad8fc7de..505bf5120c.Cancellation is handled by
exec.CommandContext().@ -435,0 +396,4 @@if options.token != "" {authHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(options.token)))gitArguments = append(gitArguments, "-c", fmt.Sprintf("http.extraHeader='%s'", authHeader))codeberg.org/forgejo/forgejo@acdfb122cb/modules/git/repo.go (L153-L181)shows how to set credentials without leaking them in the command line
Fixed in
fd2207c005..8f16371924fd2207c0058f163719248f163719248bad8fc7de@ -241,0 +180,4 @@_ = os.RemoveAll(repoDir)} else {// The remote exists and has not changed, therefore we do not have to clone the repository.return nilIf getRemoteURL fails and repoDir exists, the clone that follows will fail because repoDir exists.
That is true. And as far I know,
git clonewill complain about it. The end result is the same: an error.Or do you want me to decrease the likelihood of a clone error? The resulting code would still ignore the error, but have to perform an additional check whether the directory and exists. If it exists and
getRemoteURLfails, we can assume that the clone is unhealthy and remove it. But is it a good idea? Something went seriously wrong if we encountered that situation.@aahlenst wrote in #1255 (comment):
True. But then the git clone error will shadow the first error. I do not feel strongly about this: the previous implementation had the same "let's ignore errors" behavior and I'm fine with not changing that in this pull request.
My comment was more in reaction to never handling or displaying any of the gitRemoteURL errors which is an unusual pattern. But I'm find with how it currently is, this is a minor non blocking matter.
8bad8fc7de505bf5120ccascading-pr updated at actions/setup-forgejo#805
I haven't been able to complete my review yet, but I have pulled this branch down to run through it locally, and I've tagged for the end-to-end tests to run.
@ -641,3 +690,2 @@require.NoError(t, err)assert.Equal(t, "refs/tags/tag-1", ref)runtime.GC() // release file locks held by go-git on Windowsassert.Equal(t, "tags/tag-1", ref)I'm a little concerned about the test change here. From my understanding of the code,
FindGitRefwhich is nowDescribeHeadis used to populate${{ forgejo.ref }}, and the output here has changed fromrefs/tags/tag-1totags/tag-1. Which would seem like it would cause compatibility issues with workflows that use this var. Am I mistaken in that analysis?It's a little subtle where this is used in the runner, but I think I've identified it --
forgejo-runner execwill now report${{ forgejo.ref }}asheads/...rather thanrefs/heads/.... In non-exec cases the ref is provided by Forgejo rather than determined by the runner.Thanks a lot for finding that problem and figuring out where it's being used.
Should be fixed with
505bf5120c..7105022726.505bf5120c7105022726cascading-pr updated at actions/setup-forgejo#805
@ -437,3 +312,1 @@executor := r.NewPlanExecutor(plan)return executor(ctx)func runExec(ctx context.Context, execArgs *executeArgs) error {The only change here is the removal of the closure which makes
runExec()easier to test. The closure has been moved to the cobra configuration.@ -0,0 +38,4 @@for _, testCase := range testCases {args := &executeArgs{event: testCase.event,containerDaemonSocket: "/var/run/docker.sock",It doesn't work with Podman no matter what's specified here and at least it silences the warning.
I just realized that the test isn't included in
make integration-test, so it won't be run by CI. I guess there are more tests in the code base that aren't picked up.Having to manage paths or even to remember to update the Makefile isn't sustainable. Do you have any ideas to fix this mess, @mfenniak?
71050227269c87ac35ffcascading-pr updated at actions/setup-forgejo#805
Something like this?
Regarding testing, I can think of two reasonable approaches, with the goal that there is no risk that newly added tests aren't executed because they need to be listed in a
Makefile:Option A)
make testrunsgo test -short, andmake integration-testruns it without-short, without any package targeting at all. Asmake integration-testwould be repeating all the same tests thatmake testruns, there's probably no value in doing both in separate CI steps -- the CI should probably just lint, and then runmake integration-test.Option B) take a "filter" approach, where the Makefile would list known "integration test" (or slow test) packages that are run in
make integration-test, and everything else generated bygo list ./...is run inmake test. Forgejo almost does this, but then it still hard-codes the migration and integration tests in two places; the filter, and later in the test target that they're run, which is ... OK but not perfect.codeberg.org/forgejo/forgejo@ef6debfc02/Makefile (L289-L291)Option A is simpler... and I think that the current set up is overly complicated... so I'd lean that way. It would require touching quite a few tests to put
if testing.Short() { t.Skip("skipping integration test") }, but this pattern is already used in some tests here so it's aligned in that direction already.(Something to do in a separate change. 🙂)
Fantastic work. 👍
Thanks a lot @limiting-factor for your advice. Much appreciated.
Thanks a lot for your work on the runner.