Merge branch 'main' into bucket-provider-interface

CHANGELOG.md

@@ -2,6 +2,35 @@
 
 All notable changes to this project are documented in this file.
 
+## 0.16.1
+
+**Release date:** 2021-10-22
+
+This prerelease adds support for GCP storage authentication using the
+`GOOGLE_APPLICATION_CREDENTIALS` environment variable available in the container,
+or by defining a `secretRef` with a `serviceaccount` JSON data blob. See
+[#434](https://github.com/fluxcd/source-controller/pull/434) for more information.
+
+In addition, several bug fixes and improvements have been made to the `libgit2`
+Git implementation, ensuring the checkout logic is more rigorously tested.
+
+During this work, it was discovered that both Git implementation had a minor bug
+resulting in `v` prefixed tags with metadata added to it (e.g. `v0.1.0+build-1`
+and `v0.1.0+build-2`) were not properly sorted by their commit timestamp, which
+has been addressed as well.
+
+Improvements:
+* Add GCP storage authentication
+  [#434](https://github.com/fluxcd/source-controller/pull/434)
+
+Fixes:
+* libgit2: correctly resolve (annotated) tags
+  [#457](https://github.com/fluxcd/source-controller/pull/457)
+* libgit2: add remaining checkout strategy tests
+  [#458](https://github.com/fluxcd/source-controller/pull/458)
+* git: ensure original tag is used for TS lookup
+  [#459](https://github.com/fluxcd/source-controller/pull/459)
+
 ## 0.16.0
 
 **Release date:** 2021-10-08

config/manager/kustomization.yaml

@@ -6,4 +6,4 @@ resources:
 images:
 - name: fluxcd/source-controller
   newName: fluxcd/source-controller
-  newTag: v0.16.0
+  newTag: v0.16.1

go.mod

@@ -18,7 +18,7 @@ require (
 	github.com/fluxcd/pkg/ssh v0.1.0
 	github.com/fluxcd/pkg/untar v0.1.0
 	github.com/fluxcd/pkg/version v0.1.0
-	github.com/fluxcd/source-controller/api v0.16.0
+	github.com/fluxcd/source-controller/api v0.16.1
 	github.com/go-git/go-billy/v5 v5.3.1
 	github.com/go-git/go-git/v5 v5.4.2
 	github.com/go-logr/logr v0.4.0

pkg/git/gogit/checkout.go

@@ -212,7 +212,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 	}
 
 	var matchedVersions semver.Collection
-	for tag, _ := range tags {
+	for tag := range tags {
 		v, err := version.ParseVersion(tag)
 		if err != nil {
 			continue
@@ -239,7 +239,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		// versions into a chronological order. This is especially important for
 		// versions that differ only by build metadata, because it is not considered
 		// a part of the comparable version in Semver
-		return tagTimestamps[left.String()].Before(tagTimestamps[right.String()])
+		return tagTimestamps[left.Original()].Before(tagTimestamps[right.Original()])
 	})
 	v := matchedVersions[len(matchedVersions)-1]
 	t := v.Original()

pkg/git/libgit2/checkout.go

@@ -76,6 +76,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, auth *g
 	if err != nil {
 		return nil, "", fmt.Errorf("git resolve HEAD error: %w", err)
 	}
+	defer head.Free()
 	commit, err := repo.LookupCommit(head.Target())
 	if err != nil {
 		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target(), err)
@@ -98,31 +99,12 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, auth *git.
 		},
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
-	ref, err := repo.References.Dwim(c.tag)
+	commit, err := checkoutDetachedDwim(repo, c.tag)
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to find tag '%s': %w", c.tag, err)
+		return nil, "", err
 	}
-	err = repo.SetHeadDetached(ref.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-	head, err := repo.Head()
-	if err != nil {
-		return nil, "", fmt.Errorf("git resolve HEAD error: %w", err)
-	}
-	commit, err := repo.LookupCommit(head.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target(), err)
-	}
-	err = repo.CheckoutHead(&git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", c.tag, commit.Id().String()), nil
 }
 
@@ -140,30 +122,19 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, auth *g
 				CertificateCheckCallback: auth.CertCallback,
 			},
 		},
-		CheckoutBranch: c.branch,
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
+
 	oid, err := git2go.NewOid(c.commit)
 	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' could not be parsed", c.commit)
+		return nil, "", fmt.Errorf("could not create oid for '%s': %w", c.commit, err)
 	}
-	commit, err := repo.LookupCommit(oid)
+	commit, err := checkoutDetachedHEAD(repo, oid)
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", c.commit, err)
-	}
-	tree, err := repo.LookupTree(commit.TreeId())
-	if err != nil {
-		return nil, "", fmt.Errorf("git worktree error: %w", err)
-	}
-	err = repo.CheckoutTree(tree, &git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
 	if err != nil {
 		return nil, "", fmt.Errorf("git checkout error: %w", err)
 	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", c.branch, commit.Id().String()), nil
 }
 
@@ -187,7 +158,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		},
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
 
 	tags := make(map[string]string)
@@ -198,6 +169,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		// Due to this, first attempt to resolve it as a simple tag (commit), but fallback to attempting to
 		// resolve it as an annotated tag in case this results in an error.
 		if c, err := repo.LookupCommit(id); err == nil {
+			defer c.Free()
 			// Use the commit metadata as the decisive timestamp.
 			tagTimestamps[cleanName] = c.Committer().When
 			tags[cleanName] = name
@@ -207,14 +179,17 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		if err != nil {
 			return fmt.Errorf("could not lookup '%s' as simple or annotated tag: %w", cleanName, err)
 		}
+		defer t.Free()
 		commit, err := t.Peel(git2go.ObjectCommit)
 		if err != nil {
 			return fmt.Errorf("could not get commit for tag '%s': %w", t.Name(), err)
 		}
+		defer commit.Free()
 		c, err := commit.AsCommit()
 		if err != nil {
 			return fmt.Errorf("could not get commit object for tag '%s': %w", t.Name(), err)
 		}
+		defer c.Free()
 		tagTimestamps[t.Name()] = c.Committer().When
 		tags[t.Name()] = name
 		return nil
@@ -250,33 +225,67 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		// versions into a chronological order. This is especially important for
 		// versions that differ only by build metadata, because it is not considered
 		// a part of the comparable version in Semver
-		return tagTimestamps[left.String()].Before(tagTimestamps[right.String()])
+		return tagTimestamps[left.Original()].Before(tagTimestamps[right.Original()])
 	})
 	v := matchedVersions[len(matchedVersions)-1]
 	t := v.Original()
 
-	ref, err := repo.References.Dwim(t)
+	commit, err := checkoutDetachedDwim(repo, t)
-	if err != nil {
-		return nil, "", fmt.Errorf("unable to find tag '%s': %w", t, err)
-	}
-	err = repo.SetHeadDetached(ref.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-	head, err := repo.Head()
-	if err != nil {
-		return nil, "", fmt.Errorf("git resolve HEAD error: %w", err)
-	}
-	commit, err := repo.LookupCommit(head.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target().String(), err)
-	}
-	err = repo.CheckoutHead(&git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", t, commit.Id().String()), nil
 }
+
+// checkoutDetachedDwim attempts to perform a detached HEAD checkout by first DWIMing the short name
+// to get a concrete reference, and then calling checkoutDetachedHEAD.
+func checkoutDetachedDwim(repo *git2go.Repository, name string) (*git2go.Commit, error) {
+	ref, err := repo.References.Dwim(name)
+	if err != nil {
+		return nil, fmt.Errorf("unable to find '%s': %w", name, err)
+	}
+	defer ref.Free()
+	c, err := ref.Peel(git2go.ObjectCommit)
+	if err != nil {
+		return nil, fmt.Errorf("could not get commit for ref '%s': %w", ref.Name(), err)
+	}
+	defer c.Free()
+	commit, err := c.AsCommit()
+	if err != nil {
+		return nil, fmt.Errorf("could not get commit object for ref '%s': %w", ref.Name(), err)
+	}
+	defer commit.Free()
+	return checkoutDetachedHEAD(repo, commit.Id())
+}
+
+// checkoutDetachedHEAD attempts to perform a detached HEAD checkout for the given commit.
+func checkoutDetachedHEAD(repo *git2go.Repository, oid *git2go.Oid) (*git2go.Commit, error) {
+	commit, err := repo.LookupCommit(oid)
+	if err != nil {
+		return nil, fmt.Errorf("git commit '%s' not found: %w", oid.String(), err)
+	}
+	if err = repo.SetHeadDetached(commit.Id()); err != nil {
+		commit.Free()
+		return nil, fmt.Errorf("could not detach HEAD at '%s': %w", oid.String(), err)
+	}
+	if err = repo.CheckoutHead(&git2go.CheckoutOptions{
+		Strategy: git2go.CheckoutForce,
+	}); err != nil {
+		commit.Free()
+		return nil, fmt.Errorf("git checkout error: %w", err)
+	}
+	return commit, nil
+}
+
+// headCommit returns the current HEAD of the repository, or an error.
+func headCommit(repo *git2go.Repository) (*git2go.Commit, error) {
+	head, err := repo.Head()
+	if err != nil {
+		return nil, err
+	}
+	defer head.Free()
+
+	commit, err := repo.LookupCommit(head.Target())
+	if err != nil {
+		return nil, err
+	}
+
+	return commit, nil
+}

pkg/git/libgit2/checkout_test.go

@@ -31,59 +31,245 @@ import (
 	"github.com/fluxcd/source-controller/pkg/git"
 )
 
+func TestCheckoutBranch_Checkout(t *testing.T) {
+	repo, err := initBareRepo()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	firstCommit, err := commitFile(repo, "branch", "init", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = createBranch(repo, "test", nil); err != nil {
+		t.Fatal(err)
+	}
+
+	secondCommit, err := commitFile(repo, "branch", "second", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name           string
+		branch         string
+		expectedCommit string
+		expectedErr    string
+	}{
+		{
+			name:           "Default branch",
+			branch:         "master",
+			expectedCommit: secondCommit.String(),
+		},
+		{
+			name:           "Other branch",
+			branch:         "test",
+			expectedCommit: firstCommit.String(),
+		},
+		{
+			name:        "Non existing branch",
+			branch:      "invalid",
+			expectedErr: "reference 'refs/remotes/origin/invalid' not found",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			branch := CheckoutBranch{
+				branch: tt.branch,
+			}
+			tmpDir, _ := os.MkdirTemp("", "test")
+			defer os.RemoveAll(tmpDir)
+
+			_, ref, err := branch.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+			if tt.expectedErr != "" {
+				g.Expect(err.Error()).To(ContainSubstring(tt.expectedErr))
+				g.Expect(ref).To(BeEmpty())
+				return
+			}
+			g.Expect(ref).To(Equal(tt.branch + "/" + tt.expectedCommit))
+			g.Expect(err).To(BeNil())
+		})
+	}
+}
+
+func TestCheckoutTag_Checkout(t *testing.T) {
+	tests := []struct {
+		name        string
+		tag         string
+		annotated   bool
+		checkoutTag string
+		expectTag   string
+		expectErr   string
+	}{
+		{
+			name:        "Tag",
+			tag:         "tag-1",
+			checkoutTag: "tag-1",
+			expectTag:   "tag-1",
+		},
+		{
+			name:        "Annotated",
+			tag:         "annotated",
+			annotated:   true,
+			checkoutTag: "annotated",
+			expectTag:   "annotated",
+		},
+		{
+			name:        "Non existing tag",
+			checkoutTag: "invalid",
+			expectErr:   "unable to find 'invalid': no reference found for shorthand 'invalid'",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			repo, err := initBareRepo()
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var commit *git2go.Commit
+			if tt.tag != "" {
+				c, err := commitFile(repo, "tag", tt.tag, time.Now())
+				if err != nil {
+					t.Fatal(err)
+				}
+				if commit, err = repo.LookupCommit(c); err != nil {
+					t.Fatal(err)
+				}
+				_, err = tag(repo, c, !tt.annotated, tt.tag, time.Now())
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			tag := CheckoutTag{
+				tag: tt.checkoutTag,
+			}
+			tmpDir, _ := os.MkdirTemp("", "test")
+			defer os.RemoveAll(tmpDir)
+
+			_, ref, err := tag.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+			if tt.expectErr != "" {
+				g.Expect(err.Error()).To(Equal(tt.expectErr))
+				g.Expect(ref).To(BeEmpty())
+				return
+			}
+			if tt.expectTag != "" {
+				g.Expect(ref).To(Equal(tt.expectTag + "/" + commit.Id().String()))
+				g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile())
+				g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag))
+			}
+		})
+	}
+}
+
+func TestCheckoutCommit_Checkout(t *testing.T) {
+	g := NewWithT(t)
+
+	repo, err := initBareRepo()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer repo.Free()
+	defer os.RemoveAll(repo.Path())
+
+	c, err := commitFile(repo, "commit", "init", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err = commitFile(repo, "commit", "second", time.Now()); err != nil {
+		t.Fatal(err)
+	}
+
+	commit := CheckoutCommit{
+		commit: c.String(),
+		branch: "main",
+	}
+	tmpDir, _ := os.MkdirTemp("", "git2go")
+	defer os.RemoveAll(tmpDir)
+
+	_, ref, err := commit.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+	g.Expect(err).To(BeNil())
+	g.Expect(ref).To(Equal("main/" + c.String()))
+	g.Expect(filepath.Join(tmpDir, "commit")).To(BeARegularFile())
+	g.Expect(os.ReadFile(filepath.Join(tmpDir, "commit"))).To(BeEquivalentTo("init"))
+
+	commit = CheckoutCommit{
+		commit: "4dc3185c5fc94eb75048376edeb44571cece25f4",
+	}
+	tmpDir2, _ := os.MkdirTemp("", "git2go")
+	defer os.RemoveAll(tmpDir)
+
+	_, ref, err = commit.Checkout(context.TODO(), tmpDir2, repo.Path(), &git.Auth{})
+	g.Expect(err.Error()).To(HavePrefix("git checkout error: git commit '4dc3185c5fc94eb75048376edeb44571cece25f4' not found:"))
+	g.Expect(ref).To(BeEmpty())
+}
+
 func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 	g := NewWithT(t)
 	now := time.Now()
 
-	tags := []struct{
+	tags := []struct {
 		tag        string
-		simple     bool
+		annotated  bool
 		commitTime time.Time
 		tagTime    time.Time
 	}{
 		{
-			tag: "v0.0.1",
+			tag:        "v0.0.1",
-			simple: true,
+			annotated:  false,
 			commitTime: now,
 		},
 		{
-			tag: "v0.1.0+build-1",
+			tag:        "v0.1.0+build-1",
-			simple: false,
+			annotated:  true,
-			commitTime: now.Add(1 * time.Minute),
+			commitTime: now.Add(10 * time.Minute),
-			tagTime: now.Add(1 * time.Hour), // This should be ignored during TS comparisons
+			tagTime:    now.Add(2 * time.Hour), // This should be ignored during TS comparisons
 		},
 		{
-			tag: "v0.1.0+build-2",
+			tag:        "v0.1.0+build-2",
-			simple: true,
+			annotated:  false,
-			commitTime: now.Add(2 * time.Minute),
+			commitTime: now.Add(30 * time.Minute),
 		},
 		{
-			tag: "0.2.0",
+			tag:        "v0.1.0+build-3",
-			simple: false,
+			annotated:  true,
+			commitTime: now.Add(1 * time.Hour),
+			tagTime:    now.Add(1 * time.Hour), // This should be ignored during TS comparisons
+		},
+		{
+			tag:        "0.2.0",
+			annotated:  true,
 			commitTime: now,
-			tagTime: now,
+			tagTime:    now,
 		},
 	}
-	tests := []struct{
+	tests := []struct {
-		name        string
+		name       string
-		constraint  string
+		constraint string
-		expectError error
+		expectErr  error
-		expectTag   string
+		expectTag  string
 	}{
 		{
-			name: "Orders by SemVer",
+			name:       "Orders by SemVer",
 			constraint: ">0.1.0",
-			expectTag: "0.2.0",
+			expectTag:  "0.2.0",
 		},
 		{
-			name: "Orders by SemVer and timestamp",
+			name:       "Orders by SemVer and timestamp",
 			constraint: "<0.2.0",
-			expectTag: "v0.1.0+build-2",
+			expectTag:  "v0.1.0+build-3",
 		},
 		{
-			name: "Errors without match",
+			name:       "Errors without match",
 			constraint: ">=1.0.0",
-			expectError: errors.New("no match found for semver: >=1.0.0"),
+			expectErr:  errors.New("no match found for semver: >=1.0.0"),
 		},
 	}
 
@@ -94,12 +280,19 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 	defer repo.Free()
 	defer os.RemoveAll(repo.Path())
 
+	refs := make(map[string]string, len(tags))
 	for _, tt := range tags {
-		cId, err := commit(repo, "tag.txt", tt.tag, tt.commitTime)
+		ref, err := commitFile(repo, "tag", tt.tag, tt.commitTime)
 		if err != nil {
 			t.Fatal(err)
 		}
-		_, err = tag(repo, cId, tt.simple, tt.tag, tt.tagTime)
+		commit, err := repo.LookupCommit(ref)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer commit.Free()
+		refs[tt.tag] = commit.Id().String()
+		_, err = tag(repo, ref, tt.annotated, tt.tag, tt.tagTime)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -111,6 +304,8 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
 			semVer := CheckoutSemVer{
 				semVer: tt.constraint,
 			}
@@ -118,16 +313,15 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 			defer os.RemoveAll(tmpDir)
 
 			_, ref, err := semVer.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
-			if tt.expectError != nil {
+			if tt.expectErr != nil {
-				g.Expect(err).To(Equal(tt.expectError))
+				g.Expect(err).To(Equal(tt.expectErr))
 				g.Expect(ref).To(BeEmpty())
 				return
 			}
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(ref).To(HavePrefix(tt.expectTag + "/"))
+			g.Expect(ref).To(Equal(tt.expectTag + "/" + refs[tt.expectTag]))
-			content, err := os.ReadFile(filepath.Join(tmpDir, "tag.txt"))
+			g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile())
-			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.expectTag))
-			g.Expect(content).To(BeEquivalentTo(tt.expectTag))
 		})
 	}
 }
@@ -145,22 +339,20 @@ func initBareRepo() (*git2go.Repository, error) {
 	return repo, nil
 }
 
-func headCommit(repo *git2go.Repository) (*git2go.Commit, error) {
+func createBranch(repo *git2go.Repository, branch string, commit *git2go.Commit) error {
-	head, err := repo.Head()
+	if commit == nil {
-	if err != nil {
+		var err error
-		return nil, err
+		commit, err = headCommit(repo)
+		if err != nil {
+			return err
+		}
+		defer commit.Free()
 	}
-	defer head.Free()
+	_, err := repo.CreateBranch(branch, commit, false)
-
+	return err
-	commit, err := repo.LookupCommit(head.Target())
-	if err != nil {
-		return nil, err
-	}
-
-	return commit, nil
 }
 
-func commit(repo *git2go.Repository, path, content string, time time.Time) (*git2go.Oid, error) {
+func commitFile(repo *git2go.Repository, path, content string, time time.Time) (*git2go.Oid, error) {
 	var parentC []*git2go.Commit
 	head, err := headCommit(repo)
 	if err == nil {
@@ -192,12 +384,12 @@ func commit(repo *git2go.Repository, path, content string, time time.Time) (*git
 		return nil, err
 	}
 
-	newTreeOID, err := index.WriteTree()
+	treeID, err := index.WriteTree()
 	if err != nil {
 		return nil, err
 	}
 
-	tree, err := repo.LookupTree(newTreeOID)
+	tree, err := repo.LookupTree(treeID)
 	if err != nil {
 		return nil, err
 	}
@@ -207,19 +399,18 @@ func commit(repo *git2go.Repository, path, content string, time time.Time) (*git
 	if err != nil {
 		return nil, err
 	}
-
 	return commit, nil
 }
 
-func tag(repo *git2go.Repository, cId *git2go.Oid, simple bool, tag string, time time.Time) (*git2go.Oid, error) {
+func tag(repo *git2go.Repository, cId *git2go.Oid, annotated bool, tag string, time time.Time) (*git2go.Oid, error) {
 	commit, err := repo.LookupCommit(cId)
 	if err != nil {
 		return nil, err
 	}
-	if simple {
+	if annotated {
-		return repo.Tags.CreateLightweight(tag, commit, false)
+		return repo.Tags.Create(tag, commit, signature(time), fmt.Sprintf("Annotated tag for %s", tag))
 	}
-	return repo.Tags.Create(tag, commit, signature(time), fmt.Sprintf("Annotated tag for %s", tag))
+	return repo.Tags.CreateLightweight(tag, commit, false)
 }
 
 func signature(time time.Time) *git2go.Signature {