Skip to content

Conversation

@polatengin
Copy link
Contributor

πŸ“₯ Pull Request

πŸ”— Related Issue(s)

Close #223

❓ What are you trying to address

When someone from outside the organization (a "fork PR") submits a pull request, we have a security measure where a maintainer must approve it by typing /allow before our tests will run.

If a maintainer forgot to approve it, the PR would still show a green checkmark βœ… and could be merged - even though NO TESTS RAN AT ALL!

This is dangerous because untested code could get into our main branch.

✨ Description of new changes

We added a new check at the end of our workflows that says:

"If we were NOT allowed to run, that's a FAILURE, not a success!"

Now the workflow behaves like this:

  • Fork PR submitted β†’ No approval yet
  • Workflow says "should_run = false"
  • All test jobs get skipped
  • Final check says "NOT ALLOWED TO RUN" and FAILS ❌
  • PR cannot be merged until a maintainer types /allow

β˜‘οΈ Checklist

  • πŸ” I have performed a self-review of my own code.
  • πŸ“ I have commented my code, particularly in hard-to-understand areas.
  • 🧹 I have run the linter and fixed any issues (if applicable).
  • πŸ“„ I have updated the documentation to reflect my changes (if necessary).

@polatengin polatengin self-assigned this Nov 5, 2025
@polatengin polatengin requested a review from a team as a code owner November 5, 2025 02:22
@polatengin polatengin added the state/skip-changelog Skip changelog entry label Nov 5, 2025
Copilot AI review requested due to automatic review settings November 5, 2025 02:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds permission checks and status validation to GitHub Actions workflows to handle fork PR restrictions. The changes ensure that workflows properly fail when not authorized to run on fork PRs and prevent false success statuses when tests are skipped due to permission issues.

  • Adds explicit "Not Allowed to Run" steps that fail with clear error messages when workflows lack permission
  • Updates failure condition logic to respect the should_run check before evaluating test results
  • Introduces a new check_pr_status job to properly validate PR status and workflow completion

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
.github/workflows/test_terraform.yml Adds permission check step and updates failure condition to respect should_run output
.github/workflows/check_pr.yml Adds new check_pr_status job with permission validation and status checking logic

mluker
mluker previously approved these changes Nov 7, 2025
Copy link
Member

@mluker mluker left a comment

Choose a reason for hiding this comment

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

πŸ‘ assuming the GHCP suggestions are addressed.

@PabloZaiden
Copy link
Contributor

/allow

@github-actions
Copy link

βœ… Test Approved

@PabloZaiden has approved running terraform tests for commit 17c0eae9a9015673c71d2d26bce876660df882a1.

Approval Details:

  • Commit SHA: 17c0eae9a9015673c71d2d26bce876660df882a1
  • Approved by: @PabloZaiden
  • Approved at: 2025-11-10T15:21:01.156Z

Important: If new commits are pushed, tests will need to be re-approved.

@PabloZaiden
Copy link
Contributor

/allow

@github-actions
Copy link

βœ… Test Approved

@PabloZaiden has approved running terraform tests for commit 17c0eae9a9015673c71d2d26bce876660df882a1.

Approval Details:

  • Commit SHA: 17c0eae9a9015673c71d2d26bce876660df882a1
  • Approved by: @PabloZaiden
  • Approved at: 2025-11-10T18:44:52.129Z

Important: If new commits are pushed, tests will need to be re-approved.

@PabloZaiden
Copy link
Contributor

@polatengin, the "/allow" command doesn't seem to be working properly. It's not triggering the expected workflows, or those workflows are not detecting the approval

@PabloZaiden
Copy link
Contributor

/allow

@github-actions
Copy link

βœ… Test Approved

@PabloZaiden has approved running terraform tests for commit 85a678279e224d795c81736dc1f9c7d40d467a84.

Approval Details:

  • Commit SHA: 85a678279e224d795c81736dc1f9c7d40d467a84
  • Approved by: @PabloZaiden
  • Approved at: 2025-11-20T18:09:16.032Z

Important: If new commits are pushed, tests will need to be re-approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/skip-changelog Skip changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR failed/skipped the test execution, but it appears as green to be merged

3 participants