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

fix(ci): Use correct check for external repositories #8000

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented Nov 26, 2023

Motivation

There's a mistake in PR #7956 where it is skipping important jobs on all PRs in CI, rather than just external PRs.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

If a checkbox isn't relevant to the PR, mark it as done.

Specifications

https://docs.github.com/en/actions/learn-github-actions/contexts#github-context

You can see the full github.event object for an external PR here:
https://github.com/ZcashFoundation/zebra/actions/runs/6997671169/job/19034900585?pr=7999#step:2:995

Solution

Use the fork field rather than trying to guess from the PR branch name

Testing

I manually checked this worked:

Since the logic is inverted for an external PR, that should also work.

Review

This is urgent because some PRs and Mergify are skipping some CI jobs.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@teor2345 teor2345 added C-bug Category: This is a bug A-devops Area: Pipelines, CI/CD and Dockerfiles P-Critical 🚑 C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Nov 26, 2023
@teor2345 teor2345 self-assigned this Nov 26, 2023
@teor2345 teor2345 requested a review from a team as a code owner November 26, 2023 20:55
@teor2345 teor2345 requested review from arya2 and removed request for a team November 26, 2023 20:55
@teor2345
Copy link
Collaborator Author

The lint error is a known issue that shouldn't block merging this PR:

Error: no schema with key or ref "http://json-schema.org/draft-07/schema#"

https://github.com/ZcashFoundation/zebra/actions/runs/6997949358/job/19035511113?pr=8000#step:5:13

@teor2345
Copy link
Collaborator Author

Failed due to #7898

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

mergify bot added a commit that referenced this pull request Nov 27, 2023
@mergify mergify bot merged commit 16cb890 into main Nov 27, 2023
149 of 150 checks passed
@mergify mergify bot deleted the fix-ci-fork branch November 27, 2023 17:57
@arya2
Copy link
Contributor

arya2 commented Nov 27, 2023

Not sure what happened here, the schema at that URL seems to be there.

Run marocchino/validate-dependabot@v2.1.0
Error: no schema with key or ref "http://json-schema.org/draft-07/schema#"

https://github.com/ZcashFoundation/zebra/actions/runs/7008453149/job/19064814803#step:5:13

@teor2345
Copy link
Collaborator Author

Not sure what happened here, the schema at that URL seems to be there.

Run marocchino/validate-dependabot@v2.1.0
Error: no schema with key or ref "http://json-schema.org/draft-07/schema#"

ZcashFoundation/zebra/actions/runs/7008453149/job/19064814803#step:5:13

It seems to be a rate-limit caused by how that action is implemented internally.

@arya2 arya2 mentioned this pull request Nov 29, 2023
44 tasks
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-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants