Skip to content
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 GITHUB_TOKEN env var to the npm install step (take 2) #19013

Merged
merged 1 commit into from Mar 20, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented Mar 19, 2019

Trying to fix the Azure build failures: https://dev.azure.com/github/Atom/_build/results?buildId=35928

@rafeca rafeca force-pushed the rafeca:add-github-token branch from 6e3dfc1 to 387babe Mar 20, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Ok, got the Azure pipelines correctly building... now it's time for Travis 😅

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Ok, it's going to be challenging to fix the issue with Travis, at least for external PRs: the main issue is that Travis does not expose env variables to external PRs for security reasons.

I think that the behaviour of Travis is correct in most of the cases: by exposing env vars to PRs somebody could create a malicious PR and get the value of any variable without any code review from a repo owner... In this case, though, the vector attack is almost non-existant: the GITHUB_TOKEN that we're using as an env var is a completely unprivileged token with only read-access to the Atom public repos which allows us to get a higher rate limit from the GitHub API than unauthenticated requests... so there's not much value on stealing that token.

In the case of Azure pipelines and AppVeyor I could expose the single GITHUB_TOKEN env var to PRs, so the only problem remains with Travis.

I see 2 possible solutions here:

Find out if we can disable Travis builds from PRs (or from everywhere)

From a previous conversation with @jasonrudolph :

We rely on Travis for Linux builds of PRs and release branches

Since we also do Linux builds on Azure right now, we may be able to disable the Travis ones, but somebody with more familiarity on our build system will know better.

Stop using vscode-ripgrep to download the ripgrep binaries

This means that we'll have to find another way to either download the binaries for each platform or build them ourselves during our CI builds.

If follow this path, we'll have to take care of a few things that vscode-ripgrep currently does for us:

  1. Build a binary for each platform (more info.
  2. Update the binaries every time there's a new version of ripgrep.
  3. Be sure that we don't include multiple ripgrep binaries for different platforms on each installation, since each binary is quite big.

I'm not sure which solution it's going to be easier to implement, but my gut tells me that's going to be the first one...

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Ups, it seems like atom.io uses the linux artifacts created by Travis builds 😞.

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Find out if we can disable Travis builds from PRs

I think this might be the way to go. A few thoughts:

  • For all PRs, we'd like to be able to verify that the changes can still build successfully on Linux. The Azure DevOps Linux builds already meet this need. (example)
  • For release branches inside the atom/atom repo (e.g., the 1.35-releases branch), we need to publish build artifacts. The Travis CI build currently performs this duty. The Azure DevOps build does not yet perform this duty. For the near term, it's probably easiest to continue having the Travis CI build perform this duty. Since this is only needed for branches inside the atom/atom repo (i.e., not from forks), these builds will have access to the protected environment variables. With that in mind, if there's a way to tell Travis CI to not build PRs from forks (and instead just let Azure DevOps run those builds), then I think that could meet our needs.
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Thanks for the response @jasonrudolph!

The Travis CI build currently performs this duty. The Azure DevOps build does not yet perform this duty.

Makes sense... Just to understand better, what we would need to do to generate linux releases from Azure DevOps is to make both github/atom.io and atom/atom-release-publisher use the artifacts generated from Azure right?

For the near term, it's probably easiest to continue having the Travis CI build perform this duty.

I agree, from what I saw the changes needed to atom.io and the release publisher website are not trivial...

With that in mind, if there's a way to tell Travis CI to not build PRs from forks (and instead just let Azure DevOps run those builds), then I think that could meet our needs.

I think that this should be possible, I'm gonna take a look at it 😄

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Just to understand better, what we would need to do to generate linux releases from Azure DevOps is to make both github/atom.io and atom/atom-release-publisher use the artifacts generated from Azure right?

@rafeca: That sounds right. As prior art, atom/maintainers-log#148 tracked the process of migrating the macOS builds from CircleCI to Azure DevOps, and most of the same concerns likely apply to migration the Linux builds from Travis CI to Azure DevOps.

github/atom.io#1267 and atom/atom-release-publisher#14 handled the atom.io and atom-release-publisher tasks respectively.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Thanks for the background! That's gonna be useful if we ever want to do that migration 😄

Something I realized also is that the atom-release-publisher logic assumes that there's a build status for each platform (code), which is currently the case:

Screen Shot 2019-03-20 at 14 36 26

If we ever want to build releases for multiple platforms from Azure we'd have to change either that logic or make Azure report a separate status for each platform.

Add GITHUB_TOKEN env var to the npm install step (take 2)
This will prevent issues when installing the `vscode-ripgrep` package

@rafeca rafeca force-pushed the rafeca:add-github-token branch from 387babe to 874016b Mar 20, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Ok, I've been able to disable Travis builds on Pull Requests and after force-pushing a commit on this one it seems that Travis doesn't pick it up anymore 😄

Once the other builds finish I'll merge this PR and check that everything is fine on master

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Ok, I've been able to disable Travis builds on Pull Requests and after force-pushing a commit on this one it seems that Travis doesn't pick it up anymore 😄

Oh, cool. Did that require a configuration change on https://travis-ci.com?

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Yup exactly, I disabled the following checkbox on the project settings for Atom:

Screen Shot 2019-03-20 at 15 02 15

@rafeca rafeca merged commit aca476c into atom:master Mar 20, 2019

2 checks passed

Atom Pull Requests #20190320.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the rafeca:add-github-token branch Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.