Skip to content

Commit ce89c10

Browse files
aktauaktau
committed
upload: make uploads more robust
As described in issue github-release#26, for some types of uploads the GitHub API tends to be flaky and fail. The result is an inconsistent state (an asset in state new, which doesn't seem useful for anything). Try hard to get rid of these assets. This costs some more API calls, but I think people prefer correctness over speed. If that still doesn't cover all cases, the -R flag (for replacing assets) should now work better (also for assets that weren't correctly uploaded). This does not solve the issue where github-release apparently can't detect a hung connection. Although it _should_ fix an upload failing on a retry of this case. Updates github-release#26.
1 parent 744a70e commit ce89c10

3 files changed

Lines changed: 86 additions & 28 deletions

File tree

‎assets.go‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,21 @@
11
package main
22

33
import (
4+
"fmt"
5+
"net/http"
46
"time"
7+
8+
"github.com/aktau/github-release/github"
59
)
610

711
const (
8-
ASSET_DOWNLOAD_URI = "/repos/%s/%s/releases/assets/%d"
12+
// GET /repos/:owner/:repo/releases/assets/:id
13+
// DELETE /repos/:owner/:repo/releases/assets/:id
14+
ASSET_URI = "/repos/%s/%s/releases/assets/%d"
15+
16+
// API: https://developer.github.com/v3/repos/releases/#list-assets-for-a-release
17+
// GET /repos/:owner/:repo/releases/:id/assets
18+
ASSET_RELEASE_LIST_URI = "/repos/%s/%s/releases/%d/assets"
919
)
1020

1121
type Asset struct {
@@ -20,13 +30,29 @@ type Asset struct {
2030
Published time.Time `json:"published_at"`
2131
}
2232

23-
// findAssetID returns the asset ID if name can be found in assets,
24-
// otherwise returns -1.
25-
func findAssetID(assets []Asset, name string) int {
33+
// findAsset returns the asset if an asset with name can be found in assets,
34+
// otherwise returns nil.
35+
func findAsset(assets []Asset, name string) *Asset {
2636
for _, asset := range assets {
2737
if asset.Name == name {
28-
return asset.Id
38+
return &asset
2939
}
3040
}
31-
return -1
41+
return nil
42+
}
43+
44+
// Delete sends a HTTP DELETE request for the given asset to Github. Returns
45+
// nil if the asset was deleted OR there was nothing to delete.
46+
func (a *Asset) Delete(user, repo, token string) error {
47+
URL := nvls(EnvApiEndpoint, github.DefaultBaseURL) +
48+
fmt.Sprintf(ASSET_URI, user, repo, a.Id)
49+
resp, err := github.DoAuthRequest("DELETE", URL, "application/json", token, nil, nil)
50+
if err != nil {
51+
return fmt.Errorf("failed to delete asset %s (ID: %d), HTTP error: %b", a.Name, a.Id, err)
52+
}
53+
defer resp.Body.Close()
54+
if resp.StatusCode != http.StatusNoContent {
55+
return fmt.Errorf("failed to delete asset %s (ID: %d), status: %s", a.Name, a.Id, resp.Status)
56+
}
57+
return nil
3258
}

‎cmd.go‎

Lines changed: 53 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,29 @@ func uploadcmd(opt Options) error {
123123
return err
124124
}
125125

126-
// If asked to replace, first delete the existing asset, if any.
127-
if assetID := findAssetID(rel.Assets, name); opt.Upload.Replace && assetID != -1 {
128-
URL := nvls(EnvApiEndpoint, github.DefaultBaseURL) +
129-
fmt.Sprintf(ASSET_DOWNLOAD_URI, user, repo, assetID)
130-
resp, err := github.DoAuthRequest("DELETE", URL, "application/json", token, nil, nil)
131-
if err != nil || resp.StatusCode != http.StatusNoContent {
132-
return fmt.Errorf("could not replace asset %s (ID: %d), deletion failed (error: %v, status: %s)",
133-
name, assetID, err, resp.Status)
126+
// If the user has attempted to upload this asset before, someone could
127+
// expect it to be present in the release struct (rel.Assets). However,
128+
// we have to separately ask for the specific assets of this release.
129+
// Reason: the assets in the Release struct do not contain incomplete
130+
// uploads (which regrettably happen often using the Github API). See
131+
// issue #26.
132+
var assets []Asset
133+
err = github.Client{Token: token, BaseURL: EnvApiEndpoint}.Get(fmt.Sprintf(ASSET_RELEASE_LIST_URI, user, repo, rel.Id), &assets)
134+
if err != nil {
135+
return err
136+
}
137+
138+
// Incomplete (failed) uploads will have their state set to new. These
139+
// assets are (AFAIK) useless in all cases. The only thing they will do
140+
// is prevent the upload of another asset of the same name. To work
141+
// around this GH API weirdness, let's just delete assets if:
142+
//
143+
// 1. Their state is new.
144+
// 2. The user explicitly asked to delete/replace the asset with -R.
145+
if asset := findAsset(assets, name); asset != nil &&
146+
(asset.State == "new" || opt.Upload.Replace) {
147+
if err := asset.Delete(user, repo, token); err != nil {
148+
return fmt.Errorf("could not replace asset: %v", err)
134149
}
135150
}
136151

@@ -148,21 +163,38 @@ func uploadcmd(opt Options) error {
148163
return fmt.Errorf("can't create upload request to %v, %v", url, err)
149164
}
150165
defer resp.Body.Close()
151-
152166
vprintln("RESPONSE:", resp)
153-
if resp.StatusCode != http.StatusCreated {
154-
if msg, err := ToMessage(resp.Body); err == nil {
155-
return fmt.Errorf("could not upload, status code (%v), %v",
167+
168+
var r io.Reader = resp.Body
169+
if VERBOSITY != 0 {
170+
r = io.TeeReader(r, os.Stderr)
171+
}
172+
var asset *Asset
173+
// For HTTP status 201 and 502, Github will return a JSON encoding of
174+
// the (partially) created asset.
175+
if resp.StatusCode == http.StatusBadGateway || resp.StatusCode == http.StatusCreated {
176+
vprintf("ASSET: ")
177+
asset = new(Asset)
178+
if err := json.NewDecoder(r).Decode(&asset); err != nil {
179+
return fmt.Errorf("upload failed (%s), could not unmarshal asset (err: %v)", resp.Status, err)
180+
}
181+
} else {
182+
vprintf("BODY: ")
183+
if msg, err := ToMessage(r); err == nil {
184+
return fmt.Errorf("could not upload, status code (%s), %v",
156185
resp.Status, msg)
157186
}
158-
return fmt.Errorf("could not upload, status code (%v)", resp.Status)
187+
return fmt.Errorf("could not upload, status code (%s)", resp.Status)
159188
}
160189

161-
if VERBOSITY != 0 {
162-
vprintf("BODY: ")
163-
if _, err := io.Copy(os.Stderr, resp.Body); err != nil {
164-
return fmt.Errorf("while reading response, %v", err)
190+
if resp.StatusCode == http.StatusBadGateway {
191+
// 502 means the upload failed, but GitHub still retains metadata
192+
// (an asset in state "new"). Attempt to delete that now since it
193+
// would clutter the list of release assets.
194+
if err := asset.Delete(user, repo, token); err != nil {
195+
return fmt.Errorf("upload failed (%s), could not delete partially uploaded asset (ID: %d, err: %v) in order to cleanly reset GH API state, please try again", resp.Status, asset.Id, err)
165196
}
197+
return fmt.Errorf("could not upload, status code (%s)", resp.Status)
166198
}
167199

168200
return nil
@@ -194,17 +226,17 @@ func downloadcmd(opt Options) error {
194226
return err
195227
}
196228

197-
assetID := findAssetID(rel.Assets, name)
198-
if assetID == -1 {
229+
asset := findAsset(rel.Assets, name)
230+
if asset == nil {
199231
return fmt.Errorf("coud not find asset named %s", name)
200232
}
201233

202234
var resp *http.Response
203235
if token == "" {
204-
// Use the regular github.com site it we don't have a token.
236+
// Use the regular github.com site if we don't have a token.
205237
resp, err = http.Get("https://github.com" + fmt.Sprintf("/%s/%s/releases/download/%s/%s", user, repo, tag, name))
206238
} else {
207-
url := nvls(EnvApiEndpoint, github.DefaultBaseURL) + fmt.Sprintf(ASSET_DOWNLOAD_URI, user, repo, assetID)
239+
url := nvls(EnvApiEndpoint, github.DefaultBaseURL) + fmt.Sprintf(ASSET_URI, user, repo, asset.Id)
208240
resp, err = github.DoAuthRequest("GET", url, "", token, map[string]string{
209241
"Accept": "application/octet-stream",
210242
}, nil)

‎github/github.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,10 +121,10 @@ func (c Client) Get(uri string, v interface{}) error {
121121
// Read the array, appending all elements to the slice.
122122
for dec.More() {
123123
it := reflect.New(t) // Interface to a valid pointer to an object of the same type as the slice elements.
124-
vprintf("OBJECT %T: %v\n", it, it)
125124
if err := dec.Decode(it.Interface()); err != nil {
126125
return err
127126
}
127+
vprintf("OBJECT %T: %v\n", it.Interface(), it)
128128
sl.Set(reflect.Append(sl, it.Elem()))
129129
}
130130
}

0 commit comments

Comments
 (0)