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

test(command-build): don't run on forked repos #1768

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

erezrokah
Copy link
Contributor

Build tests now require a valid auth token which is not available to forked PRs.
Fixes CI failures in #1758

@erezrokah erezrokah requested a review from a team as a code owner January 18, 2021 15:16
@erezrokah erezrokah requested a review from ehmicky January 18, 2021 15:16
@github-actions github-actions bot added the type: chore work needed to keep the product and development running smoothly label Jan 18, 2021
@eduardoboucas
Copy link
Member

What does this mean for external contributors that submit a PR? Are we able to run the tests "on our side" before merging?

@erezrokah
Copy link
Contributor Author

What does this mean for external contributors that submit a PR? Are we able to run the tests "on our side" before merging?

We can run those tests locally and they'll run on the default branch post merge.
Contributors should also run those tests locally, but we don't have a good way (yet) to enforce tests that require GitHub secrets.
I'm planning on adding a mock API server as a part of #1489 (comment) so maybe we could fallback to a mock API on forks?

@erezrokah erezrokah enabled auto-merge (squash) January 18, 2021 15:53
@erezrokah erezrokah disabled auto-merge January 18, 2021 15:54
@erezrokah erezrokah merged commit 2110739 into master Jan 18, 2021
@erezrokah erezrokah deleted the chore/run_build_tests_only_on_forks branch January 18, 2021 15:54
@eduardoboucas
Copy link
Member

maybe we could fallback to a mock API on forks?

Sounds good. Until we get there, perhaps there's something we can do internally to ensure we don't merge a PR from a forked repo without running the test suite locally, because going forward the PR status will be misleading and there's nothing stopping us from merging a PR that actually breaks the build.

@erezrokah
Copy link
Contributor Author

Sounds good. Until we get there, perhaps there's something we can do internally to ensure we don't merge a PR from a forked repo without running the test suite locally, because going forward the PR status will be misleading and there's nothing stopping us from merging a PR that actually breaks the build.

Good point. Let's continue this discussion in #1769

vtenfys pushed a commit to vtenfys/netlify-cli-slim that referenced this pull request Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants