search

nchammas / flintrock

A command-line tool for launching Apache Spark clusters.
Apache License 2.0
638 stars 116 forks source link

Allow users to set the git commit to install Spark from as 'latest' #98

Closed BenFradet closed 8 years ago

BenFradet commented 8 years ago

fixes #61

nchammas commented 8 years ago

Thanks for working on this @BenFradet! It looks good, and I appreciate that you added tests for the new method too. I went through a left a bunch of comments.

One last thing we should address here is what branch gets queried when we ask for the latest commit. I believe GitHub (or maybe Git?) has a concept of a default branch for a repo, so your call to the GitHub API probably translates to "the latest commit on the default branch". For some repos it may not be master.

We should just clarify this with a few extra words in the help string.

BenFradet commented 8 years ago

@nchammas thanks for your review. Will make the necessary changes later today.

BenFradet commented 8 years ago

@nchammas I think I've addressed all your comments!

nchammas commented 8 years ago

Thanks @BenFradet! This looks great. I'm going to pull it in to a new PR and make some tweaks before merging it in. Thank you for working on this and going through the rounds of review. :+1:

BenFradet commented 8 years ago

np