Skip to content

Implement fork check for release task#458

Merged
MattyTheHacker merged 9 commits intomainfrom
fork-check
Apr 14, 2025
Merged

Implement fork check for release task#458
MattyTheHacker merged 9 commits intomainfrom
fork-check

Conversation

@MattyTheHacker
Copy link
Copy Markdown
Member

No description provided.

@MattyTheHacker MattyTheHacker added the bug Something isn't working label Apr 13, 2025
@MattyTheHacker MattyTheHacker self-assigned this Apr 13, 2025
@MattyTheHacker MattyTheHacker enabled auto-merge (squash) April 13, 2025 12:43
Comment thread .github/workflows/check-build-deploy.yaml Outdated
@CarrotManMatt CarrotManMatt added the deployment Changes to the deployment or CI/CD configuration label Apr 13, 2025
@MattyTheHacker
Copy link
Copy Markdown
Member Author

i believe this should now work

Copy link
Copy Markdown
Member

@CarrotManMatt CarrotManMatt left a comment

Choose a reason for hiding this comment

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

Why does github.repository not work for the pull request event too. That way it can be simplified?

@MattyTheHacker
Copy link
Copy Markdown
Member Author

MattyTheHacker commented Apr 13, 2025

Why does github.repository not work for the pull request event too. That way it can be simplified?

Annoyingly, github.repository returns the target repository for pull requests, not the originating repo. See the logs from #408 -
image
image

I can't seem to find any documentation that explicitly states this, but it appears to be the case

@MattyTheHacker
Copy link
Copy Markdown
Member Author

however don't actually merge this yet - noticed something wrong

@MattyTheHacker
Copy link
Copy Markdown
Member Author

right that should be good now

Copy link
Copy Markdown
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.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/check-build-deploy.yaml Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@MattyTheHacker
Copy link
Copy Markdown
Member Author

wait I've just had a thought - surely there's no way a push not on this repo can trigger a workflow, right? so the push check actually isn't needed at all?

Copy link
Copy Markdown
Member

@CarrotManMatt CarrotManMatt left a comment

Choose a reason for hiding this comment

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

If a pull request is the only event that can trigger this workflow from another repo then surely we want to allow all other events in this check, not just push. This is so that we don't need to add the new event types to the triggers as well as this check in the future.

Would something like this work?

github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'CSSUoB/TeX-Bot-Py-V2'

@MattyTheHacker
Copy link
Copy Markdown
Member Author

If a pull request is the only event that can trigger this workflow from another repo

I don't know this 100% i was more hoping you would know lol, but yeah if that's defo the case the check can be simplified

@MattyTheHacker
Copy link
Copy Markdown
Member Author

If a pull request is the only event that can trigger this workflow from another repo

I don't know this 100% i was more hoping you would know lol, but yeah if that's defo the case the check can be simplified

just checked, and even if a push from an external fork could trigger a workflow, our workflow specifically only runs on pushes if it has a version tag, which a fork would (read: should) never have so have simplified the check

Copy link
Copy Markdown
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.

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

Comments suppressed due to low confidence (1)

.github/workflows/check-build-deploy.yaml:222

  • Accessing 'github.event.pull_request.head.repo.full_name' without verifying that 'pull_request' exists may lead to an evaluation error when the event is not a pull_request. Consider adding a check to ensure 'github.event.pull_request' is defined before accessing nested properties.
github.event.pull_request.head.repo.full_name == 'CSSUoB/TeX-Bot-Py-V2'

@MattyTheHacker MattyTheHacker merged commit 6eb8333 into main Apr 14, 2025
9 checks passed
@MattyTheHacker MattyTheHacker deleted the fork-check branch April 14, 2025 13:29
@CarrotManMatt
Copy link
Copy Markdown
Member

CarrotManMatt commented Apr 14, 2025

our workflow specifically only runs on pushes if it has a version tag, which a fork would (read: should) never have

Not sure I fully understand this?

  1. Our workflow runs on push of a version tag or pushes to main
  2. If somehow a fork push could trigger this workflow, that fork could still push to their main branch or push a version tag to their fork

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

Labels

bug Something isn't working deployment Changes to the deployment or CI/CD configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants