Using GitArtifact as a k8s trigger source fails when using ref

[PanagiotisS]

Describe the bug
Using GitArtifact as a k8s trigger source fails when using ref

To Reproduce
Steps to reproduce the behavior:

1. Example sensor

---
apiVersion: argoproj.io/v1alpha1
kind: Sensor
metadata:
  name: webhook-sensor
spec:
  revisionHistoryLimit: 3
  template:
    container:
      volumeMounts:
        - mountPath: /git/argoproj
          name: argoproj
      env:
        - name: DEBUG_LOG
          value: "true"
    volumes:
      - name: argoproj
        emptyDir: {}
    serviceAccountName: operate-workflow-sa
  dependencies:
    - name: gitea-dep
      eventSourceName: webhook-source
      eventName: gitea
  triggers:
    - template:
        k8s:
          operation: create
          source:
            git:
              cloneDirectory: /git/argoproj
              creds:
                password:
                  key: password
                  name: gitea-secret
                username:
                  key: username
                  name: gitea-secret
              filePath: .argo/workflow.yaml
              ref: refs/heads/feature/argo-workflow
              url: http://gitea.internal/test/test.git 
        name: workflow-trigger-with-git-source

2. Error message

webhook-sensor-sensor-9qlwv-78b95697cf-dpcqb main 2023-07-04T14:01:49.805703295+03:00 2023-07-04T11:01:49.805Z  ERROR   argo-events.sensor      sensors/listene
r.go:362        Failed to execute a trigger     {"sensorName": "webhook-sensor", "error": "failed to fetch artifact, failed after retries: failed to fetch. err
: empty git-upload-pack given", "triggerName": "workflow-trigger-with-git-source", "triggeredBy": ["gitea-dep"], "triggeredByEvents": ["35393837613432372d34373
6652d343564622d623163342d393964366438353535383038"]} 

Expected behavior
Expected to work. For example if instead of ref: refs/heads/feature/argo-workflow
I use branch: feature/argo-workflow it works. It clones the git repo, and submits the workflow without issues.

Environment (please complete the following information):

• Kubernetes: 1.26.1
• Argo: v3.5.8
• Argo Events: v1.8.0

Additional context
Related issues:

go-git: empty git-upload-pack given errors since v5.4.0 #328

related issue:
go-git: Pull: error: empty git-upload-pack given

fluxcd solution: upgrade to unreleased v5.8.0
5242551eae/go.mod (L7)

metal-robot solution: Downgrade to v5.3.0
metal-stack/metal-robot#55

Attempted Solution

Upgrading to the same un-rleased v5.8.0 as fluxcd, fixes the issue with the fetch command.

However, afterwards it tries to pull but it fails with error

webhook-sensor-sensor-bcm4w-6898d9c855-nnrzg main 2023-07-11T08:23:24.315Z      ERROR   argo-events.sensor      sensors/listener.go:362 Failed to execute a trigger     {"sensorName": "webhook-sensor", "error": "failed to fetch artifact, failed after retries: failed to pull latest updates. err: empty git-upload-pack given", "triggerName": "workflow-trigger-with-git-source", "triggeredBy": ["gitea-dep"], "triggeredByEvents": ["35623862613065362d356431372d343633322d613466372d666262396239316663343734"]}

argo-events/sensors/artifacts/git.go

Lines 180 to 194 in 46e2d01

	// In the case of a specific given ref, it shouldn't be necessary to pull
	if g.artifact.Ref != "" {
	pullOpts := &git.PullOptions{
	RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
	ReferenceName: g.getBranchOrTag().Branch,
	Force: true,
	}
	if auth != nil {
	pullOpts.Auth = auth
	}
	if err := w.Pull(pullOpts); err != nil && err != git.NoErrAlreadyUpToDate {
	return nil, fmt.Errorf("failed to pull latest updates. err: %+v", err)
	}
	}

The comment, correctly notes that is not necessary to pull.

Removing this block, we can successfully clone a repo when using ref: refs/heads/feature/argo-workflow or when using branch: feature/argo-workflow.

Steps to successfully fix:

1. git checkout v1.8.0
2. Apply the following patch

diff --git a/go.mod b/go.mod
index 6d039e7f..d250c88f 100644
--- a/go.mod
+++ b/go.mod
@@ -4,10 +4,12 @@ go 1.20
 
 retract v1.15.1 // Contains retractions only.
 
 retract v1.15.0 // Published accidentally.
 
+replace github.com/go-git/go-git/v5 => github.com/go-git/go-git/v5 v5.7.1-0.20230702134234-dd4e2b7f4b01
+
 require (
 	cloud.google.com/go/compute/metadata v0.2.3
 	cloud.google.com/go/pubsub v1.30.1
 	github.com/AdaLogics/go-fuzz-headers v0.0.0-20221103172237-443f56ff4ba8
 	github.com/Azure/azure-event-hubs-go/v3 v3.5.0
diff --git a/sensors/artifacts/git.go b/sensors/artifacts/git.go
index edabe560..ff9da0ea 100644
--- a/sensors/artifacts/git.go
+++ b/sensors/artifacts/git.go
@@ -175,25 +175,11 @@ func (g *GitArtifactReader) readFromRepository(r *git.Repository, dir string) ([
 
 	if err := w.Checkout(g.getBranchOrTag()); err != nil {
 		return nil, fmt.Errorf("failed to checkout. err: %+v", err)
 	}
 
-	// In the case of a specific given ref, it shouldn't be necessary to pull
-	if g.artifact.Ref != "" {
-		pullOpts := &git.PullOptions{
-			RecurseSubmodules: git.DefaultSubmoduleRecursionDepth,
-			ReferenceName:     g.getBranchOrTag().Branch,
-			Force:             true,
-		}
-		if auth != nil {
-			pullOpts.Auth = auth
-		}
 
-		if err := w.Pull(pullOpts); err != nil && err != git.NoErrAlreadyUpToDate {
-			return nil, fmt.Errorf("failed to pull latest updates. err: %+v", err)
-		}
-	}
 	filePath := fmt.Sprintf("%s/%s", dir, g.artifact.FilePath)
 	// symbol link is not allowed due to security concern
 	isSymbolLink, err := isSymbolLink(filePath)
 	if err != nil {
 		return nil, err

3. go mod tidy
   (this step updates a bunch of other go dependencies as well

---

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

[whynowy]

@PanagiotisS - do you have a proposal? For example, if refs: is provided, do xxxx, otherwise do xxx?

[PanagiotisS]

According to the linked issues we need to either downgrade go-git to v5.3.0 (what metal-robot did) or upgrade to the unreleased v5.8.0 (what fluxcd did). Since I am not familiar with the code (or golang) and I do not know what the impact of downgrading would be, I followed what fluxcd did and upgraded go-git (the first part of the patch I posted above).

This change affects the whole code.

With this change I was able to fetch and move on from lines 172-174 of git.go

argo-events/sensors/artifacts/git.go

Lines 172 to 174 in 46e2d01

	if err := r.Fetch(fetchOptions); err != nil && err != git.NoErrAlreadyUpToDate {
	return nil, fmt.Errorf("failed to fetch. err: %v", err)
	}

Unfortunately, I was stuck afterwards when the code was trying to pull (lines 191-193)

argo-events/sensors/artifacts/git.go

Lines 191 to 193 in 46e2d01

	if err := w.Pull(pullOpts); err != nil && err != git.NoErrAlreadyUpToDate {
	return nil, fmt.Errorf("failed to pull latest updates. err: %+v", err)
	}

However, as the comment in line 180 mentions, it works without this code-block as we fetch a specific refs so we already have what we want. Since, I am not familiar with go, I simple removed it and it worked. This change affects the code only when using refs.

Also, when not using refs (so when using branch or tag) we are only using fetch (L172-174). So, shouldn't refs be the same?

Applying the above git patch fixed the problems in my (still testing) environment.

I hope this answers your question. So my proposal is the above (git) patch I suppose. Or did you mean something different with proposal?

[github-actions]

This issue has been automatically marked as stale because it has not had
any activity in the last 60 days. It will be closed if no further activity
occurs. Thank you for your contributions.