refactor: return more errors, add more tests (#679)

adding more tests
returns same message for short SHA format like GitHub Actions
refactor error checking
add more errors :)

pkg/common/git.go

@@ -2,7 +2,6 @@ package common
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -14,10 +13,12 @@ import (
 	"sync"
 
 	git "github.com/go-git/go-git/v5"
+	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/transport/http"
 	"github.com/go-ini/ini"
 	"github.com/mattn/go-isatty"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -120,16 +121,13 @@ func findGitPrettyRef(head, root, sub string) (string, error) {
 	var name string
 	var err = filepath.Walk(filepath.Join(root, sub), func(path string, info os.FileInfo, err error) error {
 		if err != nil {
+			return err
+		}
+		if name != "" || info.IsDir() {
 			return nil
 		}
-		if name != "" {
+		var bts []byte
-			return nil
+		if bts, err = ioutil.ReadFile(path); err != nil {
-		}
-		if info.IsDir() {
-			return nil
-		}
-		bts, err := ioutil.ReadFile(path)
-		if err != nil {
 			return err
 		}
 		var pointsTo = strings.TrimSpace(string(bts))
@@ -266,13 +264,17 @@ func CloneIfRequired(ctx context.Context, refName plumbing.ReferenceName, input
 			logger.Errorf("Unable to clone %v %s: %v", input.URL, refName, err)
 			return nil, err
 		}
-		_ = os.Chmod(input.Dir, 0755)
+
+		if err = os.Chmod(input.Dir, 0755); err != nil {
+			return nil, err
+		}
 	}
 
 	return r, nil
 }
 
 // NewGitCloneExecutor creates an executor to clone git repos
+// nolint:gocyclo
 func NewGitCloneExecutor(input NewGitCloneExecutorInput) Executor {
 	return func(ctx context.Context) error {
 		logger := Logger(ctx)
@@ -288,18 +290,31 @@ func NewGitCloneExecutor(input NewGitCloneExecutorInput) Executor {
 			return err
 		}
 
-		w, err := r.Worktree()
+		// fetch latest changes
-		if err != nil {
+		err = r.Fetch(&git.FetchOptions{
+			RefSpecs: []config.RefSpec{"refs/*:refs/*", "HEAD:refs/heads/HEAD"},
+		})
+		if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) {
 			return err
 		}
 
+		var hash *plumbing.Hash
+		rev := plumbing.Revision(input.Ref)
+		if hash, err = r.ResolveRevision(rev); err != nil {
+			logger.Errorf("Unable to resolve %s: %v", input.Ref, err)
+		}
+
+		if hash.String() != input.Ref && strings.HasPrefix(hash.String(), input.Ref) {
+			return errors.Wrap(errors.New(hash.String()), "short SHA references are not supported")
+		}
+
 		// At this point we need to know if it's a tag or a branch
 		// And the easiest way to do it is duck typing
 		//
 		// If err is nil, it's a tag so let's proceed with that hash like we would if
 		// it was a sha
 		refType := "tag"
-		rev := plumbing.Revision(path.Join("refs", "tags", input.Ref))
+		rev = plumbing.Revision(path.Join("refs", "tags", input.Ref))
 		if _, err := r.Tag(input.Ref); errors.Is(err, git.ErrTagNotFound) {
 			rName := plumbing.ReferenceName(path.Join("refs", "remotes", "origin", input.Ref))
 			if _, err := r.Reference(rName, false); errors.Is(err, plumbing.ErrReferenceNotFound) {
@@ -310,60 +325,53 @@ func NewGitCloneExecutor(input NewGitCloneExecutorInput) Executor {
 				rev = plumbing.Revision(rName)
 			}
 		}
-		hash, err := r.ResolveRevision(rev)
+
-		if err != nil {
+		if hash, err = r.ResolveRevision(rev); err != nil {
 			logger.Errorf("Unable to resolve %s: %v", input.Ref, err)
 			return err
 		}
 
+		var w *git.Worktree
+		if w, err = r.Worktree(); err != nil {
+			return err
+		}
+
 		// If the hash resolved doesn't match the ref provided in a workflow then we're
 		// using a branch or tag ref, not a sha
 		//
 		// Repos on disk point to commit hashes, and need to checkout input.Ref before
 		// we try and pull down any changes
-		if hash.String() != input.Ref {
+		if hash.String() != input.Ref && refType == "branch" {
-			// Run git fetch to make sure we have the latest sha
+			logger.Debugf("Provided ref is not a sha. Checking out branch before pulling changes")
-			err := r.Fetch(&git.FetchOptions{})
+			sourceRef := plumbing.ReferenceName(path.Join("refs", "remotes", "origin", input.Ref))
-			if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) {
+			if err = w.Checkout(&git.CheckoutOptions{
-				logger.Debugf("Unable to fetch: %v", err)
+				Branch: sourceRef,
-			}
+				Force:  true,
-
+			}); err != nil {
-			if refType == "branch" {
+				logger.Errorf("Unable to checkout %s: %v", sourceRef, err)
-				logger.Debugf("Provided ref is not a sha. Checking out branch before pulling changes")
+				return err
-				sourceRef := plumbing.ReferenceName(path.Join("refs", "remotes", "origin", input.Ref))
-				err := w.Checkout(&git.CheckoutOptions{
-					Branch: sourceRef,
-					Force:  true,
-				})
-				if err != nil {
-					logger.Errorf("Unable to checkout %s: %v", sourceRef, err)
-					return err
-				}
 			}
 		}
 
-		err = w.Pull(&git.PullOptions{
+		if err = w.Pull(&git.PullOptions{
 			Force: true,
-		})
+		}); err != nil && err.Error() != "already up-to-date" {
-		if err != nil && err.Error() != "already up-to-date" {
 			logger.Debugf("Unable to pull %s: %v", refName, err)
 		}
 		logger.Debugf("Cloned %s to %s", input.URL, input.Dir)
 
-		err = w.Checkout(&git.CheckoutOptions{
+		if err = w.Checkout(&git.CheckoutOptions{
 			Hash:  *hash,
 			Force: true,
-		})
+		}); err != nil {
-		if err != nil {
 			logger.Errorf("Unable to checkout %s: %v", *hash, err)
 			return err
 		}
 
-		err = w.Reset(&git.ResetOptions{
+		if err = w.Reset(&git.ResetOptions{
 			Mode:   git.HardReset,
 			Commit: *hash,
-		})
+		}); err != nil {
-		if err != nil {
 			logger.Errorf("Unable to reset to %s: %v", hash.String(), err)
 			return err
 		}

pkg/common/git_test.go

@@ -10,6 +10,7 @@ import (
 	"syscall"
 	"testing"
 
+	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -171,44 +172,55 @@ func TestGitFindRef(t *testing.T) {
 
 func TestGitCloneExecutor(t *testing.T) {
 	for name, tt := range map[string]struct {
-		URL string
+		Err, URL, Ref string
-		Ref string
-		Err error
 	}{
 		"tag": {
+			Err: "",
 			URL: "https://github.com/actions/checkout",
 			Ref: "v2",
-			Err: nil,
 		},
 		"branch": {
+			Err: "",
 			URL: "https://github.com/anchore/scan-action",
 			Ref: "act-fails",
-			Err: nil,
 		},
 		"sha": {
+			Err: "",
 			URL: "https://github.com/actions/checkout",
 			Ref: "5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f", // v2
-			Err: nil,
+		},
+		"short-sha": {
+			Err: "short SHA references are not supported: 5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f",
+			URL: "https://github.com/actions/checkout",
+			Ref: "5a4ac90", // v2
 		},
 	} {
-		tt := tt
-		name := name
 		t.Run(name, func(t *testing.T) {
 			clone := NewGitCloneExecutor(NewGitCloneExecutorInput{
 				URL: tt.URL,
 				Ref: tt.Ref,
 				Dir: testDir(t),
 			})
+
 			err := clone(context.Background())
-			assert.ErrorIs(t, err, tt.Err)
+			if tt.Err == "" {
+				assert.Empty(t, err)
+			} else {
+				assert.EqualError(t, err, tt.Err)
+			}
 		})
 	}
 }
 
 func gitConfig() {
 	if os.Getenv("GITHUB_ACTIONS") == "true" {
-		_ = gitCmd("config", "--global", "user.email", "test@test.com")
+		var err error
-		_ = gitCmd("config", "--global", "user.name", "Unit Test")
+		if err = gitCmd("config", "--global", "user.email", "test@test.com"); err != nil {
+			log.Error(err)
+		}
+		if err = gitCmd("config", "--global", "user.name", "Unit Test"); err != nil {
+			log.Error(err)
+		}
 	}
 }
 

pkg/runner/step_context.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 
 	"github.com/kballard/go-shellquote"
+	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 
 	"github.com/nektos/act/pkg/common"
@@ -82,13 +83,17 @@ func (sc *StepContext) Executor() common.Executor {
 		}
 
 		actionDir := fmt.Sprintf("%s/%s", rc.ActionCacheDir(), strings.ReplaceAll(step.Uses, "/", "-"))
+		gitClone := common.NewGitCloneExecutor(common.NewGitCloneExecutorInput{
+			URL:   remoteAction.CloneURL(),
+			Ref:   remoteAction.Ref,
+			Dir:   actionDir,
+			Token: github.Token,
+		})
+		if err := gitClone(context.TODO()); err != nil {
+			err = errors.Cause(err)
+			return common.NewErrorExecutor(fmt.Errorf("Unable to resolve action `%s`, the provided ref `%s` is the shortened version of a commit SHA, which is not supported. Please use the full commit SHA `%s` instead", step.Uses, remoteAction.Ref, err.Error()))
+		}
 		return common.NewPipelineExecutor(
-			common.NewGitCloneExecutor(common.NewGitCloneExecutorInput{
-				URL:   remoteAction.CloneURL(),
-				Ref:   remoteAction.Ref,
-				Dir:   actionDir,
-				Token: github.Token,
-			}),
 			sc.setupAction(actionDir, remoteAction.Path),
 			sc.runAction(actionDir, remoteAction.Path),
 		)

pkg/runner/step_context_test.go

@@ -19,6 +19,9 @@ func TestStepContextExecutor(t *testing.T) {
 		{"testdata", "uses-github-noref", "push", "Expected format {org}/{repo}[/path]@ref", platforms, ""},
 		{"testdata", "uses-github-root", "push", "", platforms, ""},
 		{"testdata", "uses-github-path", "push", "", platforms, ""},
+		{"testdata", "uses-docker-url", "push", "", platforms, ""},
+		{"testdata", "uses-github-full-sha", "push", "", platforms, ""},
+		{"testdata", "uses-github-short-sha", "push", "Unable to resolve action `actions/hello-world-docker-action@b136eb8`, the provided ref `b136eb8` is the shortened version of a commit SHA, which is not supported. Please use the full commit SHA `b136eb8894c5cb1dd5807da824be97ccdf9b5423` instead", platforms, ""},
 	}
 	// These tests are sufficient to only check syntax.
 	ctx := common.WithDryrun(context.Background(), true)

pkg/runner/testdata/uses-github-full-sha/main.yml

@@ -0,0 +1,7 @@
+name: uses-github-root
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/hello-world-docker-action@b136eb8894c5cb1dd5807da824be97ccdf9b5423

pkg/runner/testdata/uses-github-short-sha/main.yml

@@ -0,0 +1,7 @@
+name: uses-github-root
+on: push
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/hello-world-docker-action@b136eb8