Merge pull request #740 from github/gh-release-improvements

Improve creating of releases

commands/release.go

@@ -8,14 +8,24 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"sync"
-	"sync/atomic"
 
+	"github.com/github/hub/Godeps/_workspace/src/github.com/octokit/go-octokit/octokit"
+	"github.com/github/hub/git"
 	"github.com/github/hub/github"
 	"github.com/github/hub/utils"
-	"github.com/github/hub/Godeps/_workspace/src/github.com/octokit/go-octokit/octokit"
 )
 
+type stringSliceValue []string
+
+func (s *stringSliceValue) Set(val string) error {
+	*s = append(*s, val)
+	return nil
+}
+
+func (s *stringSliceValue) String() string {
+	return fmt.Sprintf("%s", *s)
+}
+
 var (
 	cmdRelease = &Command{
 		Run:   release,
@@ -26,13 +36,12 @@ var (
 	cmdCreateRelease = &Command{
 		Key:   "create",
 		Run:   createRelease,
-		Usage: "release create [-d] [-p] [-a <ASSETS_DIR>] [-m <MESSAGE>|-f <FILE>] <TAG>",
+		Usage: "release create [-d] [-p] [-a <ASSETS_FILE>] [-m <MESSAGE>|-f <FILE>] <TAG>",
 		Short: "Create a new release in GitHub",
 		Long: `Creates a new release in GitHub for the project that the "origin" remote points to.
 It requires the name of the tag to release as a first argument.
 
-Specify the assets to include in the release from a directory via "-a". Without
-"-a", it finds assets from "releases/TAG" of the current directory.
+Specify the assets to include in the release via "-a".
 
 Without <MESSAGE> or <FILE>, a text editor will open in which title and body
 of the release can be entered in the same manner as git commit message.
@@ -45,15 +54,16 @@ If "-p" is given, it creates a pre-release.
 	flagReleaseDraft,
 	flagReleasePrerelease bool
 
-	flagReleaseAssetsDir,
 	flagReleaseMessage,
 	flagReleaseFile string
+
+	flagReleaseAssets stringSliceValue
 )
 
 func init() {
 	cmdCreateRelease.Flag.BoolVarP(&flagReleaseDraft, "draft", "d", false, "DRAFT")
 	cmdCreateRelease.Flag.BoolVarP(&flagReleasePrerelease, "prerelease", "p", false, "PRERELEASE")
-	cmdCreateRelease.Flag.StringVarP(&flagReleaseAssetsDir, "assets", "a", "", "ASSETS_DIR")
+	cmdCreateRelease.Flag.VarP(&flagReleaseAssets, "attach", "a", "ATTACH_ASSETS")
 	cmdCreateRelease.Flag.StringVarP(&flagReleaseMessage, "message", "m", "", "MESSAGE")
 	cmdCreateRelease.Flag.StringVarP(&flagReleaseFile, "file", "f", "", "FILE")
 
@@ -62,11 +72,11 @@ func init() {
 }
 
 func release(cmd *Command, args *Args) {
-	runInLocalRepo(func(localRepo *github.GitHubRepo, project *github.Project, gh *github.Client) {
+	runInLocalRepo(func(localRepo *github.GitHubRepo, project *github.Project, client *github.Client) {
 		if args.Noop {
 			fmt.Printf("Would request list of releases for %s\n", project)
 		} else {
-			releases, err := gh.Releases(project)
+			releases, err := client.Releases(project)
 			utils.Check(err)
 			var outputs []string
 			for _, release := range releases {
@@ -86,127 +96,155 @@ func createRelease(cmd *Command, args *Args) {
 	}
 
 	tag := args.LastParam()
+	runInLocalRepo(func(localRepo *github.GitHubRepo, project *github.Project, client *github.Client) {
+		release, err := client.Release(project, tag)
+		utils.Check(err)
 
-	assetsDir, err := getAssetsDirectory(flagReleaseAssetsDir, tag)
-	utils.Check(err)
+		if release == nil {
+			currentBranch, err := localRepo.CurrentBranch()
+			utils.Check(err)
+			branchName := currentBranch.ShortName()
 
-	runInLocalRepo(func(localRepo *github.GitHubRepo, project *github.Project, gh *github.Client) {
-		currentBranch, err := localRepo.CurrentBranch()
-		utils.Check(err)
-		branchName := currentBranch.ShortName()
+			title, body, err := getTitleAndBodyFromFlags(flagReleaseMessage, flagReleaseFile)
+			utils.Check(err)
 
-		title, body, err := getTitleAndBodyFromFlags(flagReleaseMessage, flagReleaseFile)
-		utils.Check(err)
+			var editor *github.Editor
+			if title == "" {
+				cs := git.CommentChar()
+				message, err := renderReleaseTpl(cs, tag, project.Name, branchName)
+				utils.Check(err)
+
+				editor, err = github.NewEditor("RELEASE", "release", message)
+				utils.Check(err)
 
-		if title == "" {
-			title, body, err = writeReleaseTitleAndBody(project, tag, branchName)
+				title, body, err = editor.EditTitleAndBody()
+				utils.Check(err)
+			}
+
+			params := octokit.ReleaseParams{
+				TagName:         tag,
+				TargetCommitish: branchName,
+				Name:            title,
+				Body:            body,
+				Draft:           flagReleaseDraft,
+				Prerelease:      flagReleasePrerelease,
+			}
+			release, err = client.CreateRelease(project, params)
 			utils.Check(err)
+
+			if editor != nil {
+				defer editor.DeleteFile()
+			}
 		}
 
-		params := octokit.ReleaseParams{
-			TagName:         tag,
-			TargetCommitish: branchName,
-			Name:            title,
-			Body:            body,
-			Draft:           flagReleaseDraft,
-			Prerelease:      flagReleasePrerelease}
+		if len(flagReleaseAssets) > 0 {
+			paths := make([]string, 0)
+			for _, asset := range flagReleaseAssets {
+				finder := assetFinder{}
+				p, err := finder.Find(asset)
+				utils.Check(err)
 
-		finalRelease, err := gh.CreateRelease(project, params)
-		utils.Check(err)
+				paths = append(paths, p...)
+			}
 
-		uploadReleaseAssets(gh, finalRelease, assetsDir)
+			uploader := assetUploader{
+				Client:  client,
+				Release: release,
+			}
+			err = uploader.UploadAll(paths)
+			if err != nil {
+				fmt.Println("")
+				utils.Check(err)
+			}
+		}
 
-		fmt.Printf("\n\nRelease created: %s", finalRelease.HTMLURL)
+		fmt.Printf("\n%s\n", release.HTMLURL)
 	})
 }
 
-func writeReleaseTitleAndBody(project *github.Project, tag, currentBranch string) (string, string, error) {
-	message := `
-# Creating release %s for %s from %s
-#
-# Write a message for this release. The first block
-# of text is the title and the rest is description.
-`
-	message = fmt.Sprintf(message, tag, project.Name, currentBranch)
-
-	editor, err := github.NewEditor("RELEASE", "release", message)
-	if err != nil {
-		return "", "", err
-	}
-
-	defer editor.DeleteFile()
-
-	return editor.EditTitleAndBody()
+type assetUploader struct {
+	Client  *github.Client
+	Release *octokit.Release
 }
 
-func getAssetsDirectory(assetsDir, tag string) (string, error) {
-	if assetsDir == "" {
-		pwd, err := os.Getwd()
-		utils.Check(err)
+func (a *assetUploader) UploadAll(paths []string) error {
+	errUploadChan := make(chan string)
+	successChan := make(chan bool)
+	total := len(paths)
+	count := 0
 
-		assetsDir = filepath.Join(pwd, "releases", tag)
+	for _, path := range paths {
+		go a.uploadAsync(path, successChan, errUploadChan)
 	}
 
-	if !isDir(assetsDir) {
-		return "", fmt.Errorf("The assets directory doesn't exist: %s", assetsDir)
+	a.printUploadProgress(count, total)
+
+	errUploads := make([]string, 0)
+	for {
+		select {
+		case _ = <-successChan:
+			count++
+			a.printUploadProgress(count, total)
+		case errUpload := <-errUploadChan:
+			errUploads = append(errUploads, errUpload)
+			count++
+			a.printUploadProgress(count, total)
+		}
+
+		if count == total {
+			break
+		}
 	}
 
-	if isEmptyDir(assetsDir) {
-		return "", fmt.Errorf("The assets directory is empty: %s", assetsDir)
+	var err error
+	if len(errUploads) > 0 {
+		err = fmt.Errorf("Error uploading %s", strings.Join(errUploads, ", "))
 	}
 
-	return assetsDir, nil
+	return err
 }
 
-func uploadReleaseAssets(gh *github.Client, release *octokit.Release, assetsDir string) {
-	var wg sync.WaitGroup
-	var totalAssets, countAssets uint64
-
-	filepath.Walk(assetsDir, func(path string, fi os.FileInfo, err error) error {
-		if !fi.IsDir() {
-			totalAssets += 1
-		}
-		return nil
-	})
-
-	printUploadProgress(&countAssets, totalAssets)
-
-	filepath.Walk(assetsDir, func(path string, fi os.FileInfo, err error) error {
-		if !fi.IsDir() {
-			wg.Add(1)
-
-			go func() {
-				defer func() {
-					atomic.AddUint64(&countAssets, uint64(1))
-					printUploadProgress(&countAssets, totalAssets)
-					wg.Done()
-				}()
-
-				uploadUrl, err := release.UploadURL.Expand(octokit.M{"name": fi.Name()})
-				utils.Check(err)
-
-				contentType := detectContentType(path, fi)
+func (a *assetUploader) uploadAsync(path string, successChan chan bool, errUploadChan chan string) {
+	err := a.Upload(path)
+	if err == nil {
+		successChan <- true
+	} else {
+		errUploadChan <- path
+	}
+}
 
-				file, err := os.Open(path)
-				utils.Check(err)
-				defer file.Close()
+func (a *assetUploader) Upload(path string) error {
+	contentType, err := a.detectContentType(path)
+	if err != nil {
+		return err
+	}
 
-				err = gh.UploadReleaseAsset(uploadUrl, file, contentType)
-				utils.Check(err)
-			}()
-		}
+	f, err := os.Open(path)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
 
-		return nil
-	})
+	uploadUrl, err := a.Release.UploadURL.Expand(octokit.M{"name": filepath.Base(path)})
+	if err != nil {
+		return err
+	}
 
-	wg.Wait()
+	return a.Client.UploadReleaseAsset(uploadUrl, f, contentType)
 }
 
-func detectContentType(path string, fi os.FileInfo) string {
+func (a *assetUploader) detectContentType(path string) (string, error) {
 	file, err := os.Open(path)
-	utils.Check(err)
+	if err != nil {
+		return "", err
+	}
 	defer file.Close()
 
+	fi, err := file.Stat()
+	if err != nil {
+		return "", err
+	}
+
 	fileHeader := &bytes.Buffer{}
 	headerSize := int64(512)
 	if fi.Size() < headerSize {
@@ -216,12 +254,37 @@ func detectContentType(path string, fi os.FileInfo) string {
 	// The content type detection only uses 512 bytes at most.
 	// This way we avoid copying the whole content for big files.
 	_, err = io.CopyN(fileHeader, file, headerSize)
-	utils.Check(err)
+	if err != nil {
+		return "", err
+	}
+
+	t := http.DetectContentType(fileHeader.Bytes())
 
-	return http.DetectContentType(fileHeader.Bytes())
+	return strings.Split(t, ";")[0], nil
 }
 
-func printUploadProgress(count *uint64, total uint64) {
-	out := fmt.Sprintf("Uploading assets (%d/%d)", atomic.LoadUint64(count), total)
+func (a *assetUploader) printUploadProgress(count int, total int) {
+	out := fmt.Sprintf("Uploading assets (%d/%d)", count, total)
 	fmt.Print("\r" + out)
 }
+
+type assetFinder struct {
+}
+
+func (a *assetFinder) Find(path string) ([]string, error) {
+	result := make([]string, 0)
+
+	err := filepath.Walk(path, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+
+		if !info.IsDir() {
+			result = append(result, path)
+		}
+
+		return nil
+	})
+
+	return result, err
+}

commands/release_test.go

 package commands
 
 import (
-	"github.com/github/hub/Godeps/_workspace/src/github.com/bmizerany/assert"
-	"io/ioutil"
-	"os"
-	"path/filepath"
 	"testing"
-)
-
-func TestAssetsDirWithoutFlag(t *testing.T) {
-	dir := createTempDir(t)
-	pwd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer func() {
-		os.Chdir(pwd)
-		os.RemoveAll(dir)
-	}()
 
-	os.Chdir(dir)
+	"github.com/github/hub/Godeps/_workspace/src/github.com/bmizerany/assert"
+	"github.com/github/hub/fixtures"
+)
 
-	tagDir := filepath.Join(dir, "releases", "v1.0.0")
-	assertAssetsDirSelected(t, tagDir, "")
-}
+func TestAssetFinder_Find(t *testing.T) {
+	finder := assetFinder{}
 
-func TestAssetsDirWithFlag(t *testing.T) {
-	dir := createTempDir(t)
-	defer os.RemoveAll(dir)
+	paths, err := finder.Find(fixtures.Path("release_dir", "file1"))
+	assert.Equal(t, nil, err)
+	assert.Equal(t, 1, len(paths))
 
-	tagDir := filepath.Join(dir, "releases", "v1.0.0")
-	assertAssetsDirSelected(t, tagDir, tagDir)
+	paths, err = finder.Find(fixtures.Path("release_dir", "dir"))
+	assert.Equal(t, nil, err)
+	assert.Equal(t, 3, len(paths))
 }
 
-func assertAssetsDirSelected(t *testing.T, expectedDir, flagDir string) {
-	assets, err := getAssetsDirectory(flagDir, "v1.0.0")
-	assert.NotEqual(t, nil, err) // Error if it doesn't exist
-
-	os.MkdirAll(expectedDir, 0755)
-	assets, err = getAssetsDirectory(flagDir, "v1.0.0")
-	assert.NotEqual(t, nil, err) // Error if it's empty
-
-	ioutil.TempFile(expectedDir, "gh-test")
-	assets, err = getAssetsDirectory(flagDir, "v1.0.0")
-
-	fiExpected, err := os.Stat(expectedDir)
-	fiAssets, err := os.Stat(assets)
+func TestAssetUploader_detectContentType(t *testing.T) {
+	u := &assetUploader{}
+	ct, err := u.detectContentType(fixtures.Path("release_dir", "file1"))
 
 	assert.Equal(t, nil, err)
-	assert.T(t, os.SameFile(fiExpected, fiAssets))
+	assert.Equal(t, "text/plain", ct)
 }

commands/release_tpl.go

+package commands
+
+import (
+	"bytes"
+	"text/template"
+)
+
+const releaseTmpl = `{{.CS}} Creating release {{.TagName}} for {{.ProjectName}} from {{.BranchName}}
+{{.CS}}
+{{.CS}} Write a message for this release. The first block
+{{.CS}} of text is the title and the rest is description.`
+
+type releaseMsg struct {
+	CS          string
+	TagName     string
+	ProjectName string
+	BranchName  string
+}
+
+func renderReleaseTpl(cs, tagName, projectName, branchName string) (string, error) {
+	t, err := template.New("releaseTmpl").Parse(releaseTmpl)
+	if err != nil {
+		return "", err
+	}
+
+	msg := &releaseMsg{
+		CS:          cs,
+		TagName:     tagName,
+		ProjectName: projectName,
+		BranchName:  branchName,
+	}
+
+	var b bytes.Buffer
+	err = t.Execute(&b, msg)
+
+	return b.String(), err
+}

commands/release_tpl_test.go

+package commands
+
+import (
+	"testing"
+
+	"github.com/github/hub/Godeps/_workspace/src/github.com/bmizerany/assert"
+)
+
+func TestRenderReleaseTpl(t *testing.T) {
+	msg, err := renderReleaseTpl("#", "1.0", "hub", "master")
+	assert.Equal(t, nil, err)
+
+	expMsg := `# Creating release 1.0 for hub from master
+#
+# Write a message for this release. The first block
+# of text is the title and the rest is description.`
+	assert.Equal(t, expMsg, msg)
+}

fixtures/fixtures.go

+package fixtures
+
+import (
+	"os"
+	"path/filepath"
+)
+
+func Path(segment ...string) string {
+	pwd, _ := os.Getwd()
+	p := []string{pwd, "..", "fixtures"}
+	p = append(p, segment...)
+
+	return filepath.Join(p...)
+}

github/client.go

@@ -266,6 +266,33 @@ func (client *Client) Releases(project *Project) (releases []octokit.Release, er
 	return
 }
 
+func (client *Client) Release(project *Project, tagName string) (release *octokit.Release, err error) {
+	url, err := octokit.ReleasesURL.Expand(octokit.M{"owner": project.Owner, "repo": project.Name})
+	if err != nil {
+		return
+	}
+
+	api, err := client.api()
+	if err != nil {
+		err = FormatError("getting release", err)
+		return
+	}
+
+	releases, result := api.Releases(client.requestURL(url)).All()
+	if result.HasError() {
+		err = FormatError("creating release", result.Err)
+		return
+	}
+
+	for _, release := range releases {
+		if release.TagName == tagName {
+			return &release, nil
+		}
+	}
+
+	return
+}
+
 func (client *Client) CreateRelease(project *Project, params octokit.ReleaseParams) (release *octokit.Release, err error) {
 	url, err := octokit.ReleasesURL.Expand(octokit.M{"owner": project.Owner, "repo": project.Name})
 	if err != nil {