New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add command to create releases. #129

Merged
merged 22 commits into from Dec 25, 2013

Conversation

Projects
None yet
2 participants
@calavera
Contributor

calavera commented Dec 19, 2013

This PR adds a new command to create releases with gh.

It combines two steps in a single command.

Running gh release v1.0.0 this command will perform two tasks:

  1. Create a release in your github repo for the tag v1.0.0.
  2. Upload any asset that it finds under the directory releases/v1.0.0.

The location of the assets can be modified using the flag -a.
The flags -p and -d can be passed to create a prerelease or a draft.

This command will open EDITOR if there is no message or file to get the title and the body from.

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 20, 2013

⭐️

contentType := http.DetectContentType(content)
fmt.Printf("-- Uploading %s to %s\n", contentType, uploadUrl.String())
request, err := http.NewRequest("POST", uploadUrl.String(), asset)

This comment has been minimized.

@calavera

calavera Dec 20, 2013

Contributor

@technoweenie I'm having issues uploading the asset as the body of the request, it's probably something that I'm doing wrong.

  • I pass the file as a body for the request, since it's an *io.Reader.
  • The url where I send the request is something like this: https://uploads.github.com/repos/calavera/testGoPack/releases/128568/assets?name=1.txt
  • The content type is detected as text/plain which is correct.

The api gives me a status code 422, which is "Unprocessable entity".

Any recommendation?

@calavera

This comment has been minimized.

Contributor

calavera commented Dec 21, 2013

@jingweno this is working pretty nicely now. It's a lot of code to review, so no rush.

There are a bunch of testing releases in this repo: https://github.com/calavera/testGoPack/releases

The most recents are https://github.com/calavera/testGoPack/releases/tag/v1.0.21 and https://github.com/calavera/testGoPack/releases/tag/v1.0.20

calavera added some commits Dec 21, 2013

Move `isEmptyDir` to the command utilities.
Removes duplicated test helper.
@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 21, 2013

Wow, this looks awesome! 💯 Give me a day or two to play with it

@jingweno jingweno referenced this pull request Dec 21, 2013

Closed

Add support for Releases #90

@calavera calavera referenced this pull request Dec 23, 2013

Closed

Track crashes #132

@@ -320,9 +320,11 @@ For more details, run `gh help alias`.
### gh release (beta)
$ gh release
$ gh releases

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

What do you think of merging the list and the create? Say gh release to list and gh release add to create, by following the git convention

This comment has been minimized.

@calavera

calavera Dec 23, 2013

Contributor

That's a good idea. add doesn't really sound right, maybe gh release create? or is it too long? maybe I'm just over thinking.

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

create actually sounds more accurate. Good call

}
func GetTitleAndBodyFromEditor(fn func(messageFile string) error) (title, body string, err error) {
messageFile, err := git.PullReqMsgFile()

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

It would be nice release and pull request use their own file, although the file is removed once it's used. Release could use a file called ~/.git/RELEASE_EDITMSG. Also the function in GetTitleAndBodyFromEditor seems to only init the content. How about letting GetTitleAndBodyFromEditor take the file name and the content as params directly, and it becomes GetTitleAndBodyFromEditor(fileName, content string)

This comment has been minimized.

@calavera

calavera Dec 23, 2013

Contributor

agreed

}
func uploadReleaseAssets(gh *github.Client, release *octokit.Release, assetsDir string) {
var wg sync.WaitGroup

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

We may need to take care of duplicated files upload here. On the UI, it won't allow me to upload the same file twice. Not sure whether it'll be the same for the API

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

We could prompt users for force upload duplicated file or they could ignore it

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

Oh...ignore the comments, we don't allow users to attach assets after creating a release. So it's impossible to have duplicated files.

What do you think of being able to attach assets to a release after a release has been created? We could do it in another PR. This'd be useful since a lot of times I find I draft a release and keep attaching assets before flagging the release done

This comment has been minimized.

@calavera

calavera Dec 23, 2013

Contributor

I thought about that. I think it would be better to add it in another pull request since it would be a different command.

This comment has been minimized.

@jingweno
err = gh.UploadReleaseAsset(release, file, fi, contentType)
utils.Check(err)
}()

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

This is awesome! Parallel upload!

On the other hand though, I wonder what would happen if there're more than 5 uploads going. In gh, we have 8 build artifacts need to upload.

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

I just tried to upload 8 build artifacts in a road. It hangs there keeping me as a user wondering when it'd end. It'd be nice if there's logging on the current file being uploaded. We could do it on another PR for a progress bar if we want to do something fancy 😺

_, err = io.CopyN(fileHeader, file, headerSize)
utils.Check(err)
return http.DetectContentType(fileHeader.Bytes())

This comment has been minimized.

@jingweno
return
}
func (client *Client) UploadReleaseAsset(release *octokit.Release, asset *os.File, fi os.FileInfo, contentType string) (err error) {

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

👍. We could potentially move this method to go-octokit

This comment has been minimized.

@calavera

calavera Dec 23, 2013

Contributor

Yeah, I feel like all the auth logic doesn't even make sense here. It should be handled by octokit.

utils.Check(err)
if response.StatusCode != 201 {
return fmt.Errorf("Error uploading the release asset %s, %s", fi.Name(), response.Status)

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

err = fmt.Errorf("Error uploading the release asset %s, %s", fi.Name(), response.Status) and return nothing in the end could clean up the return branches a bit, since you're using named return in this method

}
func release(cmd *Command, args *Args) {
tag := args.LastParam()

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

Need an empty check here. LastParam complains about index out of bound

defer os.RemoveAll(dir)
ioutil.TempFile(dir, "gh-utils-test-")
assert.Equal(t, false, isEmptyDir(dir))

This comment has been minimized.

@jingweno

jingweno Dec 23, 2013

Owner

How about assert.T(t, !isEmptyDir(dir))?

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 23, 2013

This is interesting. I just created a draft release with gh, the title appears as untagged-62d7a5bd55cc2a3979c6. The title should be "Gee". When you click into the release, it actually displays ok.

Is that normal? It may be a API bug or some behaviours I wasn't aware. The repo is here.

screen shot 2013-12-23 at 2 53 38 pm

screen shot 2013-12-23 at 2 56 24 pm

@calavera

This comment has been minimized.

Contributor

calavera commented Dec 23, 2013

Thanks for the review @jingweno, I'm going to address the issues today.

I think the "untagged-..." name is right for drafts, the behavior is the same when you create a draft via the UI.

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 23, 2013

@calavera Cool. Thanks for the clarification. Ya, I just realized it's the same thing I got from the UI.

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 23, 2013

@calavera Thanks for the contributions! The PR in general looks great except for some minor issues. Let me know if you need any help

calavera added some commits Dec 23, 2013

Merge branch 'master' into gh_release
* master:
  Gramma issue in README
  Bump homebrew-gh which fixes sha and download path
  Bump homebrew-gh submodule to 1.0.0
  Bump version to v1.0.0
  Make it explicit that multiple credentials are allowed
  Quote path
  Add link to available setting
  Add autoupdate setting to README
  Make sure we load the deprecated configuration and override it with the new one.
  Save the new config after assigning the credentials.
  Save the configuration with the new format when the deprecated one is detected.
  Update versions automatically when autoupdate is set to true.
@calavera

This comment has been minimized.

Contributor

calavera commented Dec 24, 2013

@jingweno I did some progress with this PR, the only things missing to change are the argument validation and the command definition, I'm going to use gh release create like we agreed.

I added a super simple progress indicator, what do you think?

gh-release-progress

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 24, 2013

@calavera I just pulled down your changes and played with it. It looks so much better now! 🤘

@calavera

This comment has been minimized.

Contributor

calavera commented Dec 24, 2013

@jingweno I'm not really sure if we should address the subcommands in this PR. After reading the arguments implementation I feel like I'd need to change quite a few things.

What do you think about handling that in another PR? We could also solve other issues there too. For instance, the flags are positional, if you set flags after the arguments they are completely ignored. I think we could handle all the issues with args/flags parsing in a different issue/PR better.

@jingweno

This comment has been minimized.

Owner

jingweno commented Dec 24, 2013

@calavera For sure, let's merge this in

@calavera

This comment has been minimized.

Contributor

calavera commented Dec 25, 2013

🆒 I'm merging this!

calavera pushed a commit that referenced this pull request Dec 25, 2013

David Calavera
Merge pull request #129 from calavera/gh_release
Add command to create releases.

@calavera calavera merged commit 9be0bb8 into jingweno:master Dec 25, 2013

1 check passed

default The Travis CI build passed
Details

@calavera calavera deleted the calavera:gh_release branch Dec 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment