kiln upload-release calculates the remote path

- Use the ReleaseManifestReader to get the release's name and version
- Build a remote path based on release name + version (i.e "${name}/${name}-${version}.tgz")
- Still validate that the path is valid for the given release source. It
  doesn't make sense to upload a release that can't be fetched from that
  source
- Change ReleaseManifestReader to use a billy filesystem (since we use
  that in our tests for upload-release

Co-authored-by: Matt Royal <mroyal@pivotal.io>
Co-authored-by: Carlos Iriarte <ciriarte@pivotal.io>

[Finishes #170710078] kiln upload-release automatically calculates the conventional remote path

Author: matt-royal

builder/release_manifest_reader.go

@@ -7,11 +7,13 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"os"
 	"path/filepath"
 	"strings"
 
-	yaml "gopkg.in/yaml.v2"
+	"gopkg.in/src-d/go-billy.v4"
+	"gopkg.in/src-d/go-billy.v4/osfs"
+
+	"gopkg.in/yaml.v2"
 )
 
 type ReleaseManifest struct {
@@ -33,14 +35,20 @@ type compiledPackage struct {
 	Stemcell string `yaml:"stemcell"`
 }
 
-type ReleaseManifestReader struct{}
+type ReleaseManifestReader struct {
+	fs billy.Filesystem
+}
 
-func NewReleaseManifestReader() ReleaseManifestReader {
-	return ReleaseManifestReader{}
+func NewReleaseManifestReader(fs billy.Filesystem) ReleaseManifestReader {
+	return ReleaseManifestReader{fs: fs}
 }
 
 func (r ReleaseManifestReader) Read(releaseTarball string) (Part, error) {
-	file, err := os.Open(releaseTarball)
+	if r.fs == nil {
+		r.fs = osfs.New("")
+	}
+
+	file, err := r.fs.Open(releaseTarball)
 	if err != nil {
 		return Part{}, err
 	}

builder/release_manifest_reader_test.go

@@ -13,6 +13,8 @@ import (
 	"path/filepath"
 	"time"
 
+	"gopkg.in/src-d/go-billy.v4/osfs"
+
 	. "github.com/pivotal-cf/kiln/builder"
 
 	. "github.com/onsi/ginkgo"
@@ -75,7 +77,7 @@ var _ = Describe("ReleaseManifestReader", func() {
 	)
 
 	BeforeEach(func() {
-		reader = NewReleaseManifestReader()
+		reader = NewReleaseManifestReader(osfs.New(""))
 		tarball, releaseSHA1 = createReleaseTarball(`
 name: release
 version: 1.2.3

commands/upload_release.go

@@ -6,6 +6,8 @@ import (
 	"log"
 	"regexp"
 
+	"github.com/pivotal-cf/kiln/builder"
+
 	"github.com/pivotal-cf/kiln/fetcher"
 	"github.com/pivotal-cf/kiln/internal/cargo"
 
@@ -34,7 +36,6 @@ type UploadRelease struct {
 
 		ReleaseSource string `short:"rs" long:"release-source" required:"true" description:"name of the release source specified in the Kilnfile"`
 		LocalPath     string `short:"lp" long:"local-path" required:"true" description:"path to BOSH release tarball"`
-		RemotePath    string `short:"rp" long:"remote-path" required:"true" description:"path at the remote source"`
 	}
 }
 
@@ -83,22 +84,31 @@ func (uploadRelease UploadRelease) Execute(args []string) error {
 		return errors.New(msg)
 	}
 
+	manifestReader := builder.NewReleaseManifestReader(uploadRelease.FS)
+	part, err := manifestReader.Read(uploadRelease.Options.LocalPath)
+	if err != nil {
+		return fmt.Errorf("error reading the release manifest: %w", err)
+	}
+
+	manifest := part.Metadata.(builder.ReleaseManifest)
+	remotePath := fmt.Sprintf("%s/%s-%s.tgz", manifest.Name, manifest.Name, manifest.Version)
+
 	re, err := regexp.Compile(rc.Regex)
 	if err != nil {
 		return fmt.Errorf("could not compile the regular expression in Kilnfile for %q: %w", rc.Bucket, err)
 	}
 
-	if !re.MatchString(uploadRelease.Options.RemotePath) {
-		return fmt.Errorf("remote-path does not match regular expression in Kilnfile for %q", rc.Bucket)
+	if !re.MatchString(remotePath) {
+		return fmt.Errorf("remote path %q does not match regular expression in Kilnfile for %q", remotePath, rc.Bucket)
 	}
 
 	uploader := uploadRelease.UploaderConfig(rc)
 
 	uploadRelease.Logger.Printf("Uploading release %q to %s as %q...\n",
-		uploadRelease.Options.LocalPath, uploadRelease.Options.ReleaseSource, uploadRelease.Options.RemotePath)
+		uploadRelease.Options.LocalPath, uploadRelease.Options.ReleaseSource, remotePath)
 	if _, err := uploader.Upload(&s3manager.UploadInput{
 		Bucket: aws.String(rc.Bucket),
-		Key:    aws.String(uploadRelease.Options.RemotePath),
+		Key:    aws.String(remotePath),
 		Body:   file,
 	}); err != nil {
 		return fmt.Errorf("upload failed: %w", err)

commands/upload_release_test.go

@@ -1,8 +1,15 @@
 package commands_test
 
 import (
-	"io/ioutil"
+	"archive/tar"
+	"compress/gzip"
+	"crypto/sha1"
+	"fmt"
+	"io"
 	"log"
+	"os"
+	"strings"
+	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -30,7 +37,7 @@ var _ = Describe("UploadRelease", func() {
 						Region:          "mars-2",
 						AccessKeyId:     "id",
 						SecretAccessKey: "secret",
-						Regex:           "^(?P<release_name>[a-z-_0-9]+)-(?P<release_version>v?[0-9\\.]+-?[a-zA-Z0-9]\\.?[0-9]*)\\.tgz$",
+						Regex:           `^\w+/(?P<release_name>[a-z-_0-9]+)-(?P<release_version>v?[0-9\.]+-?[a-zA-Z0-9]\.?[0-9]*)\.tgz$`,
 					},
 					{
 						Type: "boshio",
@@ -44,8 +51,49 @@ var _ = Describe("UploadRelease", func() {
 					},
 				}
 			}
+
+			expectedReleaseSHA string
 		)
 
+		var writeReleaseTarball = func(path, name, version string) string {
+			f, err := fs.Create(path)
+			Expect(err).NotTo(HaveOccurred())
+
+			gw := gzip.NewWriter(f)
+			tw := tar.NewWriter(gw)
+
+			releaseManifest := `
+name: ` + name + `
+version: ` + version + `
+`
+			manifestReader := strings.NewReader(releaseManifest)
+
+			header := &tar.Header{
+				Name:    "release.MF",
+				Size:    manifestReader.Size(),
+				Mode:    int64(os.O_RDONLY),
+				ModTime: time.Now(),
+			}
+			Expect(tw.WriteHeader(header)).To(Succeed())
+
+			_, err = io.Copy(tw, manifestReader)
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(tw.Close()).To(Succeed())
+			Expect(gw.Close()).To(Succeed())
+			Expect(f.Close()).To(Succeed())
+
+			tarball, err := fs.Open(path)
+			Expect(err).NotTo(HaveOccurred())
+			defer tarball.Close()
+
+			hash := sha1.New()
+			_, err = io.Copy(hash, tarball)
+			Expect(err).NotTo(HaveOccurred())
+
+			return fmt.Sprintf("%x", hash.Sum(nil))
+		}
+
 		BeforeEach(func() {
 			fs = memfs.New()
 			loader = new(fakes.KilnfileLoader)
@@ -60,19 +108,14 @@ var _ = Describe("UploadRelease", func() {
 					return nil
 				},
 			}
+			expectedReleaseSHA = writeReleaseTarball("banana-release.tgz", "banana", "1.2.3")
 		})
 
 		When("it receives a correct tarball path", func() {
 			BeforeEach(func() {
 				loader.LoadKilnfilesReturns(
 					cargo.Kilnfile{ReleaseSources: exampleReleaseSourceList()},
 					cargo.KilnfileLock{}, nil)
-
-				f, err := fs.Create("banana-release.tgz")
-				_, _ = f.Write([]byte("banana"))
-				f.Close()
-
-				Expect(err).NotTo(HaveOccurred())
 			})
 
 			It("uploads the tarball to the release source", func() {
@@ -89,28 +132,56 @@ var _ = Describe("UploadRelease", func() {
 				err := uploadRelease.Execute([]string{
 					"--kilnfile", "not-read-see-struct/Kilnfile",
 					"--local-path", "banana-release.tgz",
-					"--remote-path", "banana-release-1.2.3.tgz",
 					"--release-source", "orange-bucket",
 					"--variables-file", "my-secrets",
 				})
 
 				Expect(err).NotTo(HaveOccurred())
+
 				Expect(configUploaderCallCount).To(Equal(1))
+
 				Expect(relSrcConfig).NotTo(BeNil())
 				Expect(relSrcConfig.Bucket).To(Equal("orange-bucket"))
+
 				Expect(uploader.UploadCallCount()).To(Equal(1))
 
 				opts, fns := uploader.UploadArgsForCall(0)
-
 				Expect(fns).To(HaveLen(0))
-
 				Expect(opts.Bucket).NotTo(BeNil())
 				Expect(*opts.Bucket).To(Equal("orange-bucket"))
 				Expect(opts.Key).NotTo(BeNil())
-				Expect(*opts.Key).To(Equal("banana-release-1.2.3.tgz"))
+				Expect(*opts.Key).To(Equal("banana/banana-1.2.3.tgz"))
 
-				buf, _ := ioutil.ReadAll(opts.Body)
-				Expect(string(buf)).To(Equal("banana"))
+				hash := sha1.New()
+				_, err = io.Copy(hash, opts.Body)
+				Expect(err).NotTo(HaveOccurred())
+
+				releaseSHA := fmt.Sprintf("%x", hash.Sum(nil))
+				Expect(releaseSHA).To(Equal(expectedReleaseSHA))
+			})
+		})
+
+		When("the release tarball is invalid", func() {
+			BeforeEach(func() {
+				f, err := fs.Create("invalid-release.tgz")
+				_, _ = f.Write([]byte("invalid"))
+				f.Close()
+
+				Expect(err).NotTo(HaveOccurred())
+
+				loader.LoadKilnfilesReturns(
+					cargo.Kilnfile{ReleaseSources: exampleReleaseSourceList()},
+					cargo.KilnfileLock{}, nil)
+			})
+
+			It("errors", func() {
+				err := uploadRelease.Execute([]string{
+					"--kilnfile", "not-read-see-struct/Kilnfile",
+					"--local-path", "invalid-release.tgz",
+					"--release-source", "orange-bucket",
+					"--variables-file", "my-secrets",
+				})
+				Expect(err).To(MatchError(ContainSubstring("error reading the release manifest")))
 			})
 		})
 
@@ -124,22 +195,15 @@ var _ = Describe("UploadRelease", func() {
 					ReleaseSources: relSrcList,
 				}, cargo.KilnfileLock{}, nil)
 
-				f, err := fs.Create("banana-release.tgz")
-				_, _ = f.Write([]byte("banana"))
-				f.Close()
-
 				uploadRelease.UploaderConfig = func(rsc *cargo.ReleaseSourceConfig) commands.S3Uploader {
 					return uploader
 				}
-
-				Expect(err).NotTo(HaveOccurred())
 			})
 
 			It("returns a descriptive error", func() {
 				err := uploadRelease.Execute([]string{
 					"--kilnfile", "not-read-see-struct/Kilnfile",
 					"--local-path", "banana-release.tgz",
-					"--remote-path", "banana-release-1.2.3.tgz",
 					"--release-source", "orange-bucket",
 					"--variables-file", "my-secrets",
 				})
@@ -148,82 +212,71 @@ var _ = Describe("UploadRelease", func() {
 			})
 		})
 
-		When("the remote-path does not match the regex in the release_source", func() {
+		When("the conventional remote-path does not match the regex in the release_source", func() {
 			BeforeEach(func() {
-				relSrcList := exampleReleaseSourceList()
+				relSrcList := []cargo.ReleaseSourceConfig{
+					{
+						Type:            "s3",
+						Bucket:          "orange-bucket",
+						Region:          "mars-2",
+						AccessKeyId:     "id",
+						SecretAccessKey: "secret",
+						Regex:           "^pointless-root-dir/(?P<release_name>[a-z-_0-9]+)-(?P<release_version>v?[0-9\\.]+-?[a-zA-Z0-9]\\.?[0-9]*)\\.tgz$",
+					},
+				}
 
 				loader.LoadKilnfilesReturns(cargo.Kilnfile{
 					ReleaseSources: relSrcList,
 				}, cargo.KilnfileLock{}, nil)
 
-				f, err := fs.Create("banana-release.tgz")
-				_, _ = f.Write([]byte("banana"))
-				f.Close()
-
 				uploadRelease.UploaderConfig = func(rsc *cargo.ReleaseSourceConfig) commands.S3Uploader {
 					return uploader
 				}
-
-				Expect(err).NotTo(HaveOccurred())
 			})
 
 			It("returns a descriptive error", func() {
 				err := uploadRelease.Execute([]string{
 					"--kilnfile", "not-read-see-struct/Kilnfile",
 					"--local-path", "banana-release.tgz",
-					"--remote-path", "BLA_BLA_BLA.tgz",
 					"--release-source", "orange-bucket",
 					"--variables-file", "my-secrets",
 				})
 
-				Expect(err).To(MatchError(ContainSubstring("remote-path does not match")))
+				Expect(err).To(MatchError(ContainSubstring(`remote path "banana/banana-1.2.3.tgz" does not match`)))
 			})
 		})
 
-		When("some the remote does not exist in the Kilnfile", func() {
+		When("the given release source doesn't exist", func() {
 			When("no release sources are s3 buckets", func() {
 				BeforeEach(func() {
 					loader.LoadKilnfilesReturns(cargo.Kilnfile{}, cargo.KilnfileLock{}, nil)
-
-					f, err := fs.Create("banana-release.tgz")
-					_, _ = f.Write([]byte("banana"))
-					f.Close()
-
-					Expect(err).NotTo(HaveOccurred())
 				})
 
 				It("returns an error without suggested release sources", func() {
 					err := uploadRelease.Execute([]string{
 						"--kilnfile", "not-read-see-struct/Kilnfile",
 						"--local-path", "banana-release.tgz",
-						"--remote-path", "banana-release-1.2.3.tgz",
 						"--release-source", "orange-bucket",
 						"--variables-file", "my-secrets",
 					})
 
 					Expect(err).To(MatchError(ContainSubstring("remote release source")))
 				})
 			})
-			When("at least one release source is an s3 bucket", func() {
+
+			When("some release sources are s3 buckets", func() {
 				BeforeEach(func() {
 					loader.LoadKilnfilesReturns(
 						cargo.Kilnfile{ReleaseSources: exampleReleaseSourceList()},
 						cargo.KilnfileLock{}, nil,
 					)
-
-					f, err := fs.Create("banana-release.tgz")
-					_, _ = f.Write([]byte("banana"))
-					f.Close()
-
-					Expect(err).NotTo(HaveOccurred())
 				})
 
-				It("returns an error without suggested release sources", func() {
+				It("returns an error that suggests valid release sources", func() {
 					err := uploadRelease.Execute([]string{
 						"--kilnfile", "not-read-see-struct/Kilnfile",
 						"--local-path", "banana-release.tgz",
-						"--remote-path", "banana-release-1.2.3.tgz",
-						"--release-source", "dog-bucket",
+						"--release-source", "no-such-release-source",
 						"--variables-file", "my-secrets",
 					})