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

Make PRs from external user repositories pass or skip CI jobs #4529

Closed
Tracked by #3096
teor2345 opened this issue May 29, 2022 · 13 comments · Fixed by #7956
Closed
Tracked by #3096

Make PRs from external user repositories pass or skip CI jobs #4529

teor2345 opened this issue May 29, 2022 · 13 comments · Fixed by #7956
Assignees
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug C-security Category: Security issues

Comments

@teor2345
Copy link
Contributor

teor2345 commented May 29, 2022

Motivation

Currently, Zebra's CI fails on PRs from external contributors. This can be a really confusing and negative experience for a first-time contributor.

Example: (recent examples first)
#7516
#4527

Jobs that need to be skipped

We don't want external PRs running on our Google Cloud, so we need to skip all those jobs.
We want to skip release and deployment jobs as well, because they use secret keys.

See the full list here:
#4529 (comment)

Jobs that need to be fixed

All other jobs should be fixed if they don't work for external PRs.

Designs

We can skip some tests that would otherwise fail, because Mergify will catch them when it does a full test run. But we need to disable in-place merges to make sure Mergify does a separate run.

This is more secure, because we review code, approve it, then run Mergify on it.

Related Work

Our previous attempts to fix this issue didn't work:

@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles S-needs-triage Status: A bug report needs triage P-Low ❄️ C-security Category: Security issues labels May 29, 2022
@teor2345 teor2345 mentioned this issue May 29, 2022
@teor2345 teor2345 changed the title Skip tests that require authentication on PRs from external repositories Make PRs from external repositories pass or skip CI jobs Jul 7, 2022
@teor2345
Copy link
Contributor Author

We're getting closer to the release candidate series, so this is a medium priority now.

@teor2345
Copy link
Contributor Author

Attracting external developers is not one of the goals of the release candidate, so this is a low priority.

@teor2345 teor2345 changed the title Make PRs from external repositories pass or skip CI jobs Make PRs from external user repositories pass or skip CI jobs Jan 9, 2023
@mpguerra
Copy link
Contributor

Is it just as simple as skipping the CI Docker workflow or are there others that would need to be skipped?

How will this change affect the complexity of our CI rules and/or setup?

@teor2345
Copy link
Contributor Author

Is it just as simple as skipping the CI Docker workflow or are there others that would need to be skipped?

We'll also need to skip other workflows that write to our GitHub or Docker:

  • release drafter (PR auto-labels)
  • zcash-lightwalletd (google cloud Docker image push)

And avoid sending our GitHub access token to the protoc installer, by doing one of these things:

  • just skip those entire workflows (simplest option, and a tiny amount of work)
  • skip the protoc and --all-features parts of those workflows for external users (slightly more work)
  • commit the generated files to our git repository, and have a workflow that checks they are up to date (this would take slightly more work, but simplify a lot of other workflows, and decrease our CI failure

If we decide we really want one of these workflows to run, we can do the more complicated thing for it later.

How will this change affect the complexity of our CI rules and/or setup?

If we choose the simplest option, it is:

  • change the workflow triggers for about 5 workflows by adding 2 lines of conditions
  • change the patch workflows for those workflows to run instead

If we change the patch workflows, I don't think we even need to change Mergify at all. Which keeps things a lot simpler.

@gustavovalverde
Copy link
Member

I'll be taking this into account for the redesign as we should really solve this for contributors.

@mpguerra
Copy link
Contributor

Let's try to do this in 2023 Sprint 13 if we have time

@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2023

This is a higher priority now we know QEDIT is going to start building on Zebra.

@teor2345
Copy link
Contributor Author

teor2345 commented Sep 5, 2023

Jobs that need to be skipped

We don't want external PRs running on our Google Cloud, so we need to skip all those jobs.
We want to skip release and deployment jobs as well, because they use secret keys.

Jobs that need to be fixed

All other jobs should be fixed if they don't work for external PRs.

@teor2345 teor2345 mentioned this issue Sep 10, 2023
@gustavovalverde
Copy link
Member

While researching this (as I previously made changes to allow this behavior, by removing most secrets) I recently realized there's an open discussion as GitHub variables are impeding this from happening. And reverting from variables to fix values is a no-go: https://github.com/orgs/community/discussions/44322

In the meanwhile, the "best" approach is to request reviewers to trigger the actions after validating the proposed code, which would use the reviewer permissions.
image

I made this change as a workaround, and I'll be closing this ticket as not-planned for now.

@gustavovalverde gustavovalverde closed this as not planned Won't fix, can't repro, duplicate, stale Sep 25, 2023
@teor2345
Copy link
Contributor Author

What does this look like? Is there a test PR somewhere?

@gustavovalverde
Copy link
Member

I've seen this in other open-source repos were I've contribute to. It's basically a button around here, asking for permissions to run the workflows
image

@teor2345
Copy link
Contributor Author

This fix does not work completely. Most CI runs successfully, but anything that uses Google Cloud doesn't work due to authentication issues. Let's test the next fix either before or after making it?

Screenshot 2023-10-30 at 09 40 16 Screenshot 2023-10-30 at 09 39 54

@teor2345 teor2345 reopened this Oct 29, 2023
This was referenced Oct 29, 2023
@teor2345
Copy link
Contributor Author

Here's a quick solution to this issue:

  • skip the jobs that are failing if a PR isn't from a branch in ZcashFoundation/zebra, and add patch jobs (or change their run conditions)
  • let mergify check those jobs once the PR is approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles C-bug Category: This is a bug C-security Category: Security issues
Projects
Archived in project
3 participants