search

ethereum / ethereum-org-website

Ethereum.org is a primary online resource for the Ethereum community.
https://ethereum.org/
MIT License
5.05k stars 4.79k forks source link

Netlify builds failing on fork PRs #2400

Closed samajammin closed 3 years ago

samajammin commented 3 years ago

Describe the bug

Recently we're seeing failed builds on contributor PRs. It's related to the Github API but unlike the rate limit issues in #2160, we're seeing 401 forbidden status on the Github API (same as #1472), which results in breaking builds.

Netlify removes environment variables from the builds of external contributors, e.g. from build logs

12:09:49 PM: The following environment variables have been removed from this build to keep them secret:
12:09:49 PM: - ALGOLIA_ADMIN_KEY
12:09:49 PM: - GITHUB_TOKEN_READ_ONLY_PROD
12:09:49 PM: - GATSBY_ALGOLIA_SEARCH_KEY
12:09:49 PM: - GITHUB_TOKEN_READ_ONLY_DEV
12:09:49 PM: - CROWDIN_API_KEY
12:09:49 PM: - GITHUB_TOKEN_READ_ONLY_STAGING
12:09:49 PM: - CAPTCHA_SECRET
12:09:49 PM: - DEFI_PULSE_API_KEY
12:09:49 PM: - FAUCET_PRIVATE_KEY
12:09:49 PM: - GITHUB_TOKEN
12:09:49 PM: - ETHERSCAN_API_KEY

To Reproduce

See recent contributor PRs, e.g https://github.com/ethereum/ethereum-org-website/pull/2397#partial-pull-merging (build logs) & https://github.com/ethereum/ethereum-org-website/pull/2398 (build logs).

Expected behavior

Builds should pass 😄

samajammin commented 3 years ago

I tried replicating this issue last week by opening up a PR on my fork (https://github.com/ethereum/ethereum-org-website/pull/2378) but the build passed (see logs).

samajammin commented 3 years ago

Here's the issue: https://docs.netlify.com/configure-builds/environment-variables/

I'm waiting to hear back from Netlify Support re: how to configure the sensitivity of variables - the Github API access tokens have no scope, so we should be able to set as not sensitive.

In the meantime we can configure untrusted deploys to "require approval" before building. FYI @wackerow @ryancreatescopy I've updated to this setting for now, so any PRs from forks will require us to approve before the Netlify build will start.

samajammin commented 3 years ago

Response from Netlify support:

As for marking variables as not sensitive, that would be my simply declaring them/contextualising them in your netlify.toml for deploy previews. Unfortunately, there's not a mechanism in place to declare them per se.

In your follow-up, you state that this may need to be achieved in the Netlify UI. Luckily, I remember one such discussion on how to achieve this. I can't say I've performed it myself but the source of such knowledge is a good one! In short, a bit of logic alongside your build script should make it possible to contextualise your env vars from the UI.

samajammin commented 3 years ago

So seems like we'd have to declare public version of those variables in netlify.toml 🤔 not sure if that's a viable option: https://docs.netlify.com/configure-builds/environment-variables/#sensitive-variable-policy

wackerow commented 3 years ago

@samajammin Happy to pair up with you on this, as mentioned the approval step has been resulting in batches of PRs to build, and then API rate limit issues surface after the first one or two.

samajammin commented 3 years ago

Should we just fetch this Github commit data from the client? This would require a loading state for the contributors component.