Skip to content

Add GitHub token to GitHub API calls #2270

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

Merged
merged 9 commits into from
Jun 15, 2018
Merged

Add GitHub token to GitHub API calls #2270

merged 9 commits into from
Jun 15, 2018

Conversation

montogeek
Copy link
Member

@montogeek montogeek commented Jun 15, 2018

Follow up of #1963 that actually works.
Travis scripts runs in a child process which doesn't inherit environment variables, so you have to re export them.

See https://travis-ci.org/webpack/webpack.js.org/builds/392673634?utm_source=github_status&utm_medium=notification for logs regarding rate limits, it says 5000 instead of 60. I hope this will solve our constant failed builds.

@montogeek montogeek changed the title DO NOT MERGE - Infra GitHub token Add GitHub token to GitHub API calls Jun 15, 2018
organization: organization,
suffix: suffix
},
function(err, d) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you define the error handler outside of function call and just pass it here pls? This would make it even more readable, same goes for following error handler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of the scope of this PR, I just reformatted that file, actually I should revert it.
Also, we should change how fetch script works, one is reading stdout from the previous one, making it harder to test in isolation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah dont revert, formatting is good now

@EugeneHlushko
Copy link
Member

One stylistic comment otherwise LGTM

Copy link
Member

@byzyk byzyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@montogeek montogeek merged commit 423deed into master Jun 15, 2018
@montogeek montogeek deleted the infra-github-token branch June 15, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants