Repository: goreleaser/goreleaser
Pull request #1003 (2019-04-10 00:34:58)

Fix signature path and upload
The signature Path was assumed to be at the root of dist without
taking into account the target of the original artifact. This made the
upload of signature fails with file not found errors.

The signature Name was the actual signature filename and didn't take
into account the target and would lead to conflicts when flattening
the paths for upload. This change derives the Name from the original
artifact Name instead.

<!--

Hi, thanks for contributing!

Please make sure you read our CONTRIBUTING guide.

Please fill the fields above:

-->

<!-- If applied, this commit will... -->

Fix the Path and Name of Signature artifacts

<!-- Why is this change being made? -->

The current version does not work when uploading multiple signatures for the same build with multiple goarch/goos:
- The Path is incorrect and points to `dist/filename.sig` instead of `dist/target/filename.sig`
  ```
     ⨯ release failed after 42.20s error=GitHub Releases: failed to publish artifacts: failed to upload aws-dump.asc after 10 retries: open dist/aws-dump.asc: no such file or directory
  ```
- The Name is set to the `filename.sig` and ends up conflicting on release upload when 2 signature for the same binary for 2 different targets are uploaded.
  ```
     ⨯ release failed after 40.16s error=GitHub Releases: failed to publish artifacts: failed to upload aws-dump.asc after 10 retries: POST https://uploads.github.com/repos/hamstah/awstools/releases/16661863/assets?name=aws-dump.asc: 422 Validation Failed [{Resource:ReleaseAsset Field:name Code:already_exists Message:}]
  ```


<!-- # Provide links to any relevant tickets, URLs or other resources -->

Here is a minimal yaml file that reproduces the issue for me.

```
before:
  hooks:
    - go mod download
archive:
  format: binary
sign:
  signature: "${artifact}.asc"
  artifacts: all
release:
  draft: true
builds:
  - env:
      - CGO_ENABLED=0
    main: ./aws/dump/
    binary: aws-dump
    goos:
      - linux
    goarch:
      - amd64
```


codecov-io: # [Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=h1) Report
> Merging [#1003](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=desc) into [master](https://codecov.io/gh/goreleaser/goreleaser/commit/2de196a74579f1f65ba61b0366191dded9034c28?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `93.33%`.

[![Impacted file tree graph](https://codecov.io/gh/goreleaser/goreleaser/pull/1003/graphs/tree.svg?width=650&token=3bJvv6b7hN&height=150&src=pr)](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
+ Coverage   88.12%   88.12%   +<.01%     
==========================================
  Files          52       52              
  Lines        2517     2518       +1     
==========================================
+ Hits         2218     2219       +1     
  Misses        236      236              
  Partials       63       63
```


| [Impacted Files](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/pipe/sign/sign.go](https://codecov.io/gh/goreleaser/goreleaser/pull/1003/diff?src=pr&el=tree#diff-aW50ZXJuYWwvcGlwZS9zaWduL3NpZ24uZ28=) | `93.75% <93.33%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=footer). Last update [2de196a...beddd35](https://codecov.io/gh/goreleaser/goreleaser/pull/1003?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

caarlos0: awesome, thanks @hamstah 
can you add a test case for it, please?

thanks!
hamstah: @caarlos0 done

Patch:
diff --git a/internal/pipe/sign/sign.go b/internal/pipe/sign/sign.go
index 40691624..aa024687 100644
--- a/internal/pipe/sign/sign.go
+++ b/internal/pipe/sign/sign.go
@@ -62,30 +62,23 @@ func (Pipe) Run(ctx *context.Context) error {
 }
 
 func sign(ctx *context.Context, artifacts []artifact.Artifact) error {
-	// nolint:prealloc
-	var sigs []string
+
 	for _, a := range artifacts {
-		sig, err := signone(ctx, a)
+		artifact, err := signone(ctx, a)
 		if err != nil {
 			return err
 		}
-		sigs = append(sigs, sig)
-	}
-	for _, sig := range sigs {
-		ctx.Artifacts.Add(artifact.Artifact{
-			Type: artifact.Signature,
-			Name: sig,
-			Path: filepath.Join(ctx.Config.Dist, sig),
-		})
+
+		ctx.Artifacts.Add(*artifact)
 	}
 	return nil
 }
 
-func signone(ctx *context.Context, artifact artifact.Artifact) (string, error) {
+func signone(ctx *context.Context, a artifact.Artifact) (*artifact.Artifact, error) {
 	cfg := ctx.Config.Sign
 
 	env := map[string]string{
-		"artifact": artifact.Path,
+		"artifact": a.Path,
 	}
 	env["signature"] = expand(cfg.Signature, env)
 
@@ -103,9 +96,20 @@ func signone(ctx *context.Context, artifact artifact.Artifact) (string, error) {
 	log.WithField("cmd", cmd.Args).Debug("running")
 	output, err := cmd.CombinedOutput()
 	if err != nil {
-		return "", fmt.Errorf("sign: %s failed with %q", cfg.Cmd, string(output))
+		return nil, fmt.Errorf("sign: %s failed with %q", cfg.Cmd, string(output))
 	}
-	return filepath.Base(env["signature"]), nil
+
+	artifactPathBase, _ := filepath.Split(a.Path)
+
+	env["artifact"] = a.Name
+	name := expand(cfg.Signature, env)
+
+	sigFilename := filepath.Base(env["signature"])
+	return &artifact.Artifact{
+		Type: artifact.Signature,
+		Name: name,
+		Path: filepath.Join(artifactPathBase, sigFilename),
+	}, nil
 }
 
 func expand(s string, env map[string]string) string {


Test patch:
diff --git a/internal/pipe/sign/sign_test.go b/internal/pipe/sign/sign_test.go
index 5da11566..a8f5a270 100644
--- a/internal/pipe/sign/sign_test.go
+++ b/internal/pipe/sign/sign_test.go
@@ -70,9 +70,10 @@ func TestSignInvalidArtifacts(t *testing.T) {
 
 func TestSignArtifacts(t *testing.T) {
 	tests := []struct {
-		desc       string
-		ctx        *context.Context
-		signatures []string
+		desc           string
+		ctx            *context.Context
+		signaturePaths []string
+		signatureNames []string
 	}{
 		{
 			desc: "sign all artifacts",
@@ -81,7 +82,8 @@ func TestSignArtifacts(t *testing.T) {
 					Sign: config.Sign{Artifacts: "all"},
 				},
 			),
-			signatures: []string{"artifact1.sig", "artifact2.sig", "checksum.sig"},
+			signaturePaths: []string{"artifact1.sig", "artifact2.sig", "artifact3.sig", "checksum.sig", "linux_amd64/artifact4.sig"},
+			signatureNames: []string{"artifact1.sig", "artifact2.sig", "artifact3_1.0.0_linux_amd64.sig", "checksum.sig", "artifact4_1.0.0_linux_amd64.sig"},
 		},
 		{
 			desc: "sign only checksums",
@@ -90,20 +92,21 @@ func TestSignArtifacts(t *testing.T) {
 					Sign: config.Sign{Artifacts: "checksum"},
 				},
 			),
-			signatures: []string{"checksum.sig"},
+			signaturePaths: []string{"checksum.sig"},
+			signatureNames: []string{"checksum.sig"},
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.desc, func(tt *testing.T) {
-			testSign(tt, test.ctx, test.signatures)
+			testSign(tt, test.ctx, test.signaturePaths, test.signatureNames)
 		})
 	}
 }
 
 const user = "nopass"
 
-func testSign(t *testing.T, ctx *context.Context, signatures []string) {
+func testSign(t *testing.T, ctx *context.Context, signaturePaths []string, signatureNames []string) {
 	// create temp dir for file and signature
 	tmpdir, err := ioutil.TempDir("", "goreleaser")
 	assert.NoError(t, err)
@@ -112,11 +115,14 @@ func testSign(t *testing.T, ctx *context.Context, signatures []string) {
 	ctx.Config.Dist = tmpdir
 
 	// create some fake artifacts
-	var artifacts = []string{"artifact1", "artifact2", "checksum"}
+	var artifacts = []string{"artifact1", "artifact2", "artifact3", "checksum"}
+	os.Mkdir(filepath.Join(tmpdir, "linux_amd64"), os.ModePerm)
 	for _, f := range artifacts {
 		file := filepath.Join(tmpdir, f)
 		assert.NoError(t, ioutil.WriteFile(file, []byte("foo"), 0644))
 	}
+	assert.NoError(t, ioutil.WriteFile(filepath.Join(tmpdir, "linux_amd64", "artifact4"), []byte("foo"), 0644))
+	artifacts = append(artifacts, "linux_amd64/artifact4")
 	ctx.Artifacts.Add(artifact.Artifact{
 		Name: "artifact1",
 		Path: filepath.Join(tmpdir, "artifact1"),
@@ -127,11 +133,21 @@ func testSign(t *testing.T, ctx *context.Context, signatures []string) {
 		Path: filepath.Join(tmpdir, "artifact2"),
 		Type: artifact.UploadableArchive,
 	})
+	ctx.Artifacts.Add(artifact.Artifact{
+		Name: "artifact3_1.0.0_linux_amd64",
+		Path: filepath.Join(tmpdir, "artifact3"),
+		Type: artifact.UploadableBinary,
+	})
 	ctx.Artifacts.Add(artifact.Artifact{
 		Name: "checksum",
 		Path: filepath.Join(tmpdir, "checksum"),
 		Type: artifact.Checksum,
 	})
+	ctx.Artifacts.Add(artifact.Artifact{
+		Name: "artifact4_1.0.0_linux_amd64",
+		Path: filepath.Join(tmpdir, "linux_amd64", "artifact4"),
+		Type: artifact.UploadableBinary,
+	})
 
 	// configure the pipeline
 	// make sure we are using the test keyring
@@ -142,18 +158,31 @@ func testSign(t *testing.T, ctx *context.Context, signatures []string) {
 	assert.NoError(t, Pipe{}.Run(ctx))
 
 	// verify that only the artifacts and the signatures are in the dist dir
-	files, err := ioutil.ReadDir(tmpdir)
-	assert.NoError(t, err)
 	gotFiles := []string{}
-	for _, f := range files {
-		gotFiles = append(gotFiles, f.Name())
-	}
-	wantFiles := append(artifacts, signatures...)
+
+	err = filepath.Walk(tmpdir,
+		func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			if info.IsDir() {
+				return nil
+			}
+			relPath, err := filepath.Rel(tmpdir, path)
+			if err != nil {
+				return err
+			}
+			gotFiles = append(gotFiles, relPath)
+			return nil
+		})
+	assert.NoError(t, err)
+
+	wantFiles := append(artifacts, signaturePaths...)
 	sort.Strings(wantFiles)
 	assert.Equal(t, wantFiles, gotFiles)
 
 	// verify the signatures
-	for _, sig := range signatures {
+	for _, sig := range signaturePaths {
 		verifySignature(t, ctx, sig)
 	}
 
@@ -162,7 +191,7 @@ func testSign(t *testing.T, ctx *context.Context, signatures []string) {
 		signArtifacts = append(signArtifacts, sig.Name)
 	}
 	// check signature is an artifact
-	assert.Equal(t, signArtifacts, signatures)
+	assert.Equal(t, signArtifacts, signatureNames)
 }
 
 func verifySignature(t *testing.T, ctx *context.Context, sig string) {