[release] Fail fast when attempting to attach unavailable files

Fixes #2315

commands/release.go

@@ -450,6 +450,10 @@ func createRelease(cmd *Command, args *Args) {
 		return
 	}
 
+	assetsToUpload, close, err := openAssetFiles(args.Flag.AllValues("--attach"))
+	utils.Check(err)
+	defer close()
+
 	localRepo, err := github.LocalRepo()
 	utils.Check(err)
 
@@ -512,8 +516,25 @@ text is the title and the rest is the description.`, tagName, project))
 
 	messageBuilder.Cleanup()
 
-	flagReleaseAssets := args.Flag.AllValues("--attach")
-	uploadAssets(gh, release, flagReleaseAssets, args)
+	numAssets := len(assetsToUpload)
+	if numAssets == 0 {
+		return
+	}
+	if args.Noop {
+		ui.Printf("Would attach %d %s\n", numAssets, pluralize(numAssets, "asset"))
+	} else {
+		ui.Errorf("Attaching %d %s...\n", numAssets, pluralize(numAssets, "asset"))
+		uploaded, err := gh.UploadReleaseAssets(release, assetsToUpload)
+		if err != nil {
+			failed := []string{}
+			for _, a := range assetsToUpload[len(uploaded):] {
+				failed = append(failed, fmt.Sprintf("-a %s", a.Name))
+			}
+			ui.Errorf("The release was created, but attaching %d %s failed. ", len(failed), pluralize(len(failed), "asset"))
+			ui.Errorf("You can retry with:\n%s release edit %s -m '' %s\n\n", "hub", release.TagName, strings.Join(failed, " "))
+			utils.Check(err)
+		}
+	}
 }
 
 func editRelease(cmd *Command, args *Args) {
@@ -526,6 +547,10 @@ func editRelease(cmd *Command, args *Args) {
 		return
 	}
 
+	assetsToUpload, close, err := openAssetFiles(args.Flag.AllValues("--attach"))
+	utils.Check(err)
+	defer close()
+
 	localRepo, err := github.LocalRepo()
 	utils.Check(err)
 
@@ -585,6 +610,7 @@ text is the title and the rest is the description.`, tagName, project))
 		params["body"] = body
 	}
 
+	args.NoForward()
 	if len(params) > 0 {
 		if args.Noop {
 			ui.Printf("Would edit release `%s'\n", tagName)
@@ -596,9 +622,24 @@ text is the title and the rest is the description.`, tagName, project))
 		messageBuilder.Cleanup()
 	}
 
-	flagReleaseAssets := args.Flag.AllValues("--attach")
-	uploadAssets(gh, release, flagReleaseAssets, args)
-	args.NoForward()
+	numAssets := len(assetsToUpload)
+	if numAssets == 0 {
+		return
+	}
+	if args.Noop {
+		ui.Printf("Would attach %d %s\n", numAssets, pluralize(numAssets, "asset"))
+	} else {
+		ui.Errorf("Attaching %d %s...\n", numAssets, pluralize(numAssets, "asset"))
+		uploaded, err := gh.UploadReleaseAssets(release, assetsToUpload)
+		if err != nil {
+			failed := []string{}
+			for _, a := range assetsToUpload[len(uploaded):] {
+				failed = append(failed, a.Name)
+			}
+			ui.Errorf("Attaching these assets failed:\n%s\n\n", strings.Join(failed, "\n"))
+			utils.Check(err)
+		}
+	}
 }
 
 func deleteRelease(cmd *Command, args *Args) {
@@ -633,32 +674,48 @@ func deleteRelease(cmd *Command, args *Args) {
 	args.NoForward()
 }
 
-func uploadAssets(gh *github.Client, release *github.Release, assets []string, args *Args) {
-	for _, asset := range assets {
+func openAssetFiles(args []string) ([]github.LocalAsset, func(), error) {
+	assets := []github.LocalAsset{}
+	files := []*os.File{}
+
+	for _, arg := range args {
 		var label string
-		parts := strings.SplitN(asset, "#", 2)
-		asset = parts[0]
+		parts := strings.SplitN(arg, "#", 2)
+		path := parts[0]
 		if len(parts) > 1 {
 			label = parts[1]
 		}
 
-		if args.Noop {
-			if label == "" {
-				ui.Errorf("Would attach release asset `%s'\n", asset)
-			} else {
-				ui.Errorf("Would attach release asset `%s' with label `%s'\n", asset, label)
-			}
-		} else {
-			for _, existingAsset := range release.Assets {
-				if existingAsset.Name == filepath.Base(asset) {
-					err := gh.DeleteReleaseAsset(&existingAsset)
-					utils.Check(err)
-					break
-				}
-			}
-			ui.Errorf("Attaching release asset `%s'...\n", asset)
-			_, err := gh.UploadReleaseAsset(release, asset, label)
-			utils.Check(err)
+		file, err := os.Open(path)
+		if err != nil {
+			return nil, nil, err
+		}
+		stat, err := file.Stat()
+		if err != nil {
+			return nil, nil, err
+		}
+		files = append(files, file)
+
+		assets = append(assets, github.LocalAsset{
+			Name:     path,
+			Label:    label,
+			Size:     stat.Size(),
+			Contents: file,
+		})
+	}
+
+	close := func() {
+		for _, f := range files {
+			f.Close()
 		}
 	}
+
+	return assets, close, nil
+}
+
+func pluralize(count int, label string) string {
+	if count == 1 {
+		return label
+	}
+	return fmt.Sprintf("%ss", label)
 }

features/release.feature

@@ -453,7 +453,52 @@ MARKDOWN
     Then the output should contain exactly:
       """
       https://github.com/mislav/will_paginate/releases/v1.2.0
-      Attaching release asset `./hello-1.2.0.tar.gz'...\n
+      Attaching 1 asset...\n
+      """
+
+  Scenario: Create a release with some assets failing
+    Given the GitHub API server:
+      """
+      post('/repos/mislav/will_paginate/releases') {
+        status 201
+        json :tag_name => "v1.2.0",
+             :html_url => "https://github.com/mislav/will_paginate/releases/v1.2.0",
+             :upload_url => "https://uploads.github.com/uploads/assets{?name,label}"
+      }
+      post('/uploads/assets', :host_name => 'uploads.github.com') {
+        halt 422 if params[:name] == "two"
+        status 201
+      }
+      """
+    And a file named "one" with:
+      """
+      ONE
+      """
+    And a file named "two" with:
+      """
+      TWO
+      """
+    And a file named "three" with:
+      """
+      THREE
+      """
+    When I run `hub release create -m "m" v1.2.0 -a one -a two -a three`
+    Then the exit status should be 1
+    Then the stderr should contain exactly:
+      """
+      Attaching 3 assets...
+      The release was created, but attaching 2 assets failed. You can retry with:
+      hub release edit v1.2.0 -m '' -a two -a three
+      
+      Error uploading release asset: Unprocessable Entity (HTTP 422)\n
+      """
+
+  Scenario: Create a release with nonexistent asset
+    When I run `hub release create -m "hello" v1.2.0 -a "idontexis.tgz"`
+    Then the exit status should be 1
+    Then the stderr should contain exactly:
+      """
+      open idontexis.tgz: no such file or directory\n
       """
 
   Scenario: Open new release in web browser
@@ -586,7 +631,7 @@ MARKDOWN
     When I successfully run `hub release edit -m "" v1.2.0 -a hello-1.2.0.tar.gz`
     Then the output should contain exactly:
       """
-      Attaching release asset `hello-1.2.0.tar.gz'...\n
+      Attaching 1 asset...\n
       """
 
   Scenario: Edit release no tag

github/client.go

@@ -380,26 +380,53 @@ func (client *Client) DeleteRelease(release *Release) (err error) {
 	return
 }
 
-func (client *Client) UploadReleaseAsset(release *Release, filename, label string) (asset *ReleaseAsset, err error) {
+type LocalAsset struct {
+	Name     string
+	Label    string
+	Contents io.Reader
+	Size     int64
+}
+
+func (client *Client) UploadReleaseAssets(release *Release, assets []LocalAsset) (doneAssets []*ReleaseAsset, err error) {
 	api, err := client.simpleApi()
 	if err != nil {
 		return
 	}
 
-	parts := strings.SplitN(release.UploadUrl, "{", 2)
-	uploadUrl := parts[0]
-	uploadUrl += "?name=" + url.QueryEscape(filepath.Base(filename))
-	if label != "" {
-		uploadUrl += "&label=" + url.QueryEscape(label)
+	idx := strings.Index(release.UploadUrl, "{")
+	uploadURL := release.UploadUrl[0:idx]
+
+	for _, asset := range assets {
+		for _, existingAsset := range release.Assets {
+			if existingAsset.Name == asset.Name {
+				if err = client.DeleteReleaseAsset(&existingAsset); err != nil {
+					return
+				}
+				break
+			}
+		}
+
+		params := map[string]interface{}{"name": filepath.Base(asset.Name)}
+		if asset.Label != "" {
+			params["label"] = asset.Label
+		}
+		uploadPath := addQuery(uploadURL, params)
+
+		// TODO: retry failed assets
+		var res *simpleResponse
+		res, err = api.PostFile(uploadPath, asset.Contents, asset.Size)
+		if err = checkStatus(201, "uploading release asset", res, err); err != nil {
+			return
+		}
+
+		newAsset := ReleaseAsset{}
+		err = res.Unmarshal(&newAsset)
+		if err != nil {
+			return
+		}
+		doneAssets = append(doneAssets, &newAsset)
 	}
 
-	res, err := api.PostFile(uploadUrl, filename)
-	if err = checkStatus(201, "uploading release asset", res, err); err != nil {
-		return
-	}
-
-	asset = &ReleaseAsset{}
-	err = res.Unmarshal(asset)
 	return
 }
 

github/http.go

@@ -434,20 +434,11 @@ func (c *simpleClient) PatchJSON(path string, payload interface{}) (*simpleRespo
 	return c.jsonRequest("PATCH", path, payload, nil)
 }
 
-func (c *simpleClient) PostFile(path, filename string) (*simpleResponse, error) {
-	stat, err := os.Stat(filename)
-	if err != nil {
-		return nil, err
-	}
-
-	file, err := os.Open(filename)
-	if err != nil {
-		return nil, err
-	}
-	defer file.Close()
-
-	return c.performRequest("POST", path, file, func(req *http.Request) {
-		req.ContentLength = stat.Size()
+func (c *simpleClient) PostFile(path string, contents io.Reader, fileSize int64) (*simpleResponse, error) {
+	return c.performRequest("POST", path, contents, func(req *http.Request) {
+		if fileSize > 0 {
+			req.ContentLength = fileSize
+		}
 		req.Header.Set("Content-Type", "application/octet-stream")
 	})
 }