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

Adds support to configure the GitHub access token to be used for GitHub API calls #605

Merged
merged 6 commits into from Jul 14, 2015

Conversation

Projects
None yet
5 participants
@guidomb
Contributor

guidomb commented Jul 7, 2015

fixes #603

To avoid Github rate limit issues now it is possible
to provide a Github API access token.

In shared environment where several virtual machines
are using the same public ip address (like travis ci),
carthage user could hit a Github API rate limit. By
providing a Github API access token, carthage can get
a higher rate limit.

Adds --github-access-token checkout option.
To avoid Github rate limit issues now it is possible
to provide a Github API access token.

In shared environment where several virtual machines
are using the same public ip address (like travis ci),
carthage user could hit a Github API rate limit. By
providing a Github API access token, carthage can get
a higher rate limit.
@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Jul 7, 2015

Contributor

You can check https://github.com/Wolox/ReactiveArray to see a running version of this patch in TravisCI

Contributor

guidomb commented Jul 7, 2015

You can check https://github.com/Wolox/ReactiveArray to see a running version of this patch in TravisCI

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 8, 2015

Member

I have mixed feels about this. @Carthage/carthage?

Member

mdiep commented Jul 8, 2015

I have mixed feels about this. @Carthage/carthage?

@robrix

This comment has been minimized.

Show comment
Hide comment
@robrix

robrix Jul 8, 2015

Contributor

Seems like maybe this shouldn’t be a command line argument, but maybe an environment variable?

Contributor

robrix commented Jul 8, 2015

Seems like maybe this shouldn’t be a command line argument, but maybe an environment variable?

@robb

This comment has been minimized.

Show comment
Hide comment
@robb

robb Jul 8, 2015

Member

The need is real, but I agree that an environment variable is probably the way to go here.

Member

robb commented Jul 8, 2015

The need is real, but I agree that an environment variable is probably the way to go here.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 8, 2015

Member

👍 to an environment variable.

Member

mdiep commented Jul 8, 2015

👍 to an environment variable.

@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Jul 8, 2015

Contributor

I will remove the command line parameter and add support for the GITHUB_ACCESS_TOKEN env variable.

I wasn't sure about adding a the githubAccessToken parameter to this and this method. It just doesn't feel right but by adding the env variable all the logic needed to decided the authentication method can be encapuslated in the Github.swift file.

Should I amend the previous commit or add a new one?

Contributor

guidomb commented Jul 8, 2015

I will remove the command line parameter and add support for the GITHUB_ACCESS_TOKEN env variable.

I wasn't sure about adding a the githubAccessToken parameter to this and this method. It just doesn't feel right but by adding the env variable all the logic needed to decided the authentication method can be encapuslated in the Github.swift file.

Should I amend the previous commit or add a new one?

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 8, 2015

Member

New commits are preferred to rewriting history. 😄

Member

mdiep commented Jul 8, 2015

New commits are preferred to rewriting history. 😄

@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Jul 8, 2015

Contributor

Just replaced the command line argument for the env variable. Let me know if you want to change something else.

Contributor

guidomb commented Jul 8, 2015

Just replaced the command line argument for the env variable. Let me know if you want to change something else.

@guidomb guidomb changed the title from Adds --github-access-token checkout option. to Adds support to configure the GitHub access token to be used for GitHub API calls Jul 8, 2015

Show outdated Hide outdated Source/CarthageKit/GitHub.swift Outdated
Show outdated Hide outdated Source/CarthageKit/GitHub.swift Outdated
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 13, 2015

Member

I really like where this is headed—thanks for working on it! And sorry for the delayed review.

Member

mdiep commented Jul 13, 2015

I really like where this is headed—thanks for working on it! And sorry for the delayed review.

guidomb added some commits Jul 13, 2015

Reads GITHUB_ACCESS_TOKEN env variable inside CarthageKit.
Instead of reading the env variable in the Carthage command
line application we do it inside CarthageKit.
@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Jul 14, 2015

Contributor

@mdiep I've just commited the fixes we were discussing. Let me know if there is anything else. Should I amend all the commits before merge?

Contributor

guidomb commented Jul 14, 2015

@mdiep I've just commited the fixes we were discussing. Let me know if there is anything else. Should I amend all the commits before merge?

Show outdated Hide outdated Source/CarthageKit/GitHub.swift Outdated
Show outdated Hide outdated Source/CarthageKit/Project.swift Outdated
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 14, 2015

Member

Should I amend all the commits before merge?

No, you don't need to amend the commits. We prefer history to be unaltered.

Member

mdiep commented Jul 14, 2015

Should I amend all the commits before merge?

No, you don't need to amend the commits. We prefer history to be unaltered.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 14, 2015

Member

Thanks for all the work you put into this. It looks great!

Member

mdiep commented Jul 14, 2015

Thanks for all the work you put into this. It looks great!

mdiep added a commit that referenced this pull request Jul 14, 2015

Merge pull request #605 from guidomb/github-access-token-option
Adds support to configure the GitHub access token to be used for GitHub API calls

@mdiep mdiep merged commit d38ddcb into Carthage:master Jul 14, 2015

@ikesyo

This comment has been minimized.

Show comment
Hide comment
@ikesyo

ikesyo Jul 14, 2015

Member

🎉

Member

ikesyo commented Jul 14, 2015

🎉

@guidomb guidomb deleted the guidomb:github-access-token-option branch Jul 14, 2015

@robb

This comment has been minimized.

Show comment
Hide comment
@robb

robb Aug 5, 2015

Member

@mdiep would you mind building a new release including this feature?

Member

robb commented Aug 5, 2015

@mdiep would you mind building a new release including this feature?

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Aug 5, 2015

Member

@robb I'll try to do so in the next few days. I've been busy with a move, but would really like to get all the new bits out in the wild.

Member

mdiep commented Aug 5, 2015

@robb I'll try to do so in the next few days. I've been busy with a move, but would really like to get all the new bits out in the wild.

@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Aug 5, 2015

Contributor

@mdiep I would be nice (if possible) to have #583 in the next release too. Thanks!

Contributor

guidomb commented Aug 5, 2015

@mdiep I would be nice (if possible) to have #583 in the next release too. Thanks!

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Aug 7, 2015

Member

I just released 0.8: Llama Calculus. #583 didn't make it in, but I'll cut another release once the current batch of PRs is merged.

I haven't opened a PR for homebrew since I don't use it, but it'd be if someone wanted to do so.

Member

mdiep commented Aug 7, 2015

I just released 0.8: Llama Calculus. #583 didn't make it in, but I'll cut another release once the current batch of PRs is merged.

I haven't opened a PR for homebrew since I don't use it, but it'd be if someone wanted to do so.

@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb

guidomb Aug 7, 2015

Contributor

@mdiep To open a PR for homebrew do we need to collaborators or anybody can do it? I've never released a brew formula before.

Contributor

guidomb commented Aug 7, 2015

@mdiep To open a PR for homebrew do we need to collaborators or anybody can do it? I've never released a brew formula before.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Aug 7, 2015

Member

I think anybody, but I've never done it either. You should be able to search for Carthage in their PRs to find an example.

Member

mdiep commented Aug 7, 2015

I think anybody, but I've never done it either. You should be able to search for Carthage in their PRs to find an example.

@guidomb

This comment has been minimized.

Show comment
Hide comment
@guidomb
Contributor

guidomb commented Aug 7, 2015

guidomb added a commit to guidomb/ios-scripts that referenced this pull request Aug 13, 2015

Removes the use of custom Carthage build.
Carthage version 0.8 has been released and there is no need to
use a custom build because
Carthage/Carthage#605 is available now

adamkaplan added a commit to yahoo/YMCache that referenced this pull request Sep 10, 2015

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