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

[Github] Add repository checks to release-binaries workflow #84437

Conversation

boomanaiden154
Copy link
Contributor

This patch adds repository checks to the release-binaries workflow jobs. People were observing that the job was running on a schedule in their forks. This only happens on old forks, but those probably exist in great number given how prolific LLVM is. This is also good practice anyways, on top of solving the direct problem of these jobs running with the cron schedule on people's forks.

This patch adds repository checks to the release-binaries workflow jobs.
People were observing that the job was running on a schedule in their
forks. This only happens on old forks, but those probably exist in great
number given how prolific LLVM is. This is also good practice anyways,
on top of solving the direct problem of these jobs running with the cron
schedule on people's forks.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-github-workflow

Author: Aiden Grossman (boomanaiden154)

Changes

This patch adds repository checks to the release-binaries workflow jobs. People were observing that the job was running on a schedule in their forks. This only happens on old forks, but those probably exist in great number given how prolific LLVM is. This is also good practice anyways, on top of solving the direct problem of these jobs running with the cron schedule on people's forks.


Full diff: https://github.com/llvm/llvm-project/pull/84437.diff

1 Files Affected:

  • (modified) .github/workflows/release-binaries.yml (+3)
diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml
index ebf6fa41898dc4..e487b0f1d4b5d6 100644
--- a/.github/workflows/release-binaries.yml
+++ b/.github/workflows/release-binaries.yml
@@ -35,6 +35,7 @@ jobs:
   prepare:
     name: Prepare to build binaries
     runs-on: ubuntu-22.04
+    if: github.repository == 'llvm/llvm-project'
     outputs:
       release-version: ${{ steps.vars.outputs.release-version }}
       flags: ${{ steps.vars.outputs.flags }}
@@ -85,6 +86,7 @@ jobs:
     name: "Fill Cache ${{ matrix.os }}"
     needs: prepare
     runs-on: ${{ matrix.os }}
+    if: github.repository == 'llvm/llvm-project'
     strategy:
       matrix:
         os:
@@ -119,6 +121,7 @@ jobs:
       - prepare
       - fill-cache
     runs-on: ${{ matrix.target.runs-on }}
+    if: github.repository == 'llvm/llvm-project'
     strategy:
       fail-fast: false
       matrix:

@mstorsjo
Copy link
Member

mstorsjo commented Mar 8, 2024

People were observing that the job was running on a schedule in their forks. This only happens on old forks, but those probably exist in great number given how prolific LLVM is.

Can you elaborate on what mechanisms are involved here? I remember seeing something on this topic somewhere, but don't remember the specifics. Forks of repos made after date X don't run scheduled actions automatically, unless fork user does Y to enable scheduled actions. Or did this extend to any kind of actions, e.g. ones with a regular push: trigger as well?

@boomanaiden154
Copy link
Contributor Author

Can you elaborate on what mechanisms are involved here? I remember seeing something on this topic somewhere, but don't remember the specifics. Forks of repos made after date X don't run scheduled actions automatically, unless fork user does Y to enable scheduled actions. Or did this extend to any kind of actions, e.g. ones with a regular push: trigger as well?

I believe it should be after a certain date (and that's what I remember). I can't find anything explicit in the documentation mentioning this though. There is just this paragraph:

Warning: To prevent unnecessary workflow runs, scheduled workflows may be disabled automatically. When a public repository is forked, scheduled workflows are disabled by default. In a public repository, scheduled workflows are automatically disabled when no repository activity has occurred in 60 days.

There's also this screen that shows up on the actions tab when forking at least some repositories recently:

image

I can't find anything in the documentation about it though. From what I understand, scheduled workflows are disabled by default, but other workflow events are enabled by default (might have changed at some point though). Given the age of many LLVM forks though, a lot of workflows will just run, including scheduled workflows (hence this patch). We've found it's just good practice to restrict the repository jobs will run on to avoid this issue.

@mstorsjo
Copy link
Member

mstorsjo commented Mar 8, 2024

Warning: To prevent unnecessary workflow runs, scheduled workflows may be disabled automatically. When a public repository is forked, scheduled workflows are disabled by default. In a public repository, scheduled workflows are automatically disabled when no repository activity has occurred in 60 days.

There's also this screen that shows up on the actions tab when forking at least some repositories recently:

image

I can't find anything in the documentation about it though. From what I understand, scheduled workflows are disabled by default, but other workflow events are enabled by default (might have changed at some point though). Given the age of many LLVM forks though, a lot of workflows will just run, including scheduled workflows (hence this patch). We've found it's just good practice to restrict the repository jobs will run on to avoid this issue.

Hmm, right. It's a bit non-obvious that there's a distinction between whether a repo did have actions at the point when it was forked or not, but I guess it makes sense. Thinking further, it would be nice if there was an explicit setting that one can set on one's fork, to disable/enable scheduled actions...

But in any case, restricting the scheduled actions explicitly certainly is best in any case. So +1 from me too.

@boomanaiden154
Copy link
Contributor Author

Hmm, right. It's a bit non-obvious that there's a distinction between whether a repo did have actions at the point when it was forked or not, but I guess it makes sense. Thinking further, it would be nice if there was an explicit setting that one can set on one's fork, to disable/enable scheduled actions...

Definitely. I wish it was better documented (or that I was better at finding the documentation if it exists). There are explicit settings that do exist.

https://docs.github.com/en/actions/using-workflows/disabling-and-enabling-a-workflow shows how to enable and disable specific workflows. This requires knowing what workflows are interesting to disable though.

By going into settings->actions->disable actions, you can also disable all actions in your fork. I believe this shouldn't impact any upstream contributions as the pull_request target should be from the workflow definition in the target branch.

@boomanaiden154 boomanaiden154 merged commit 9f5be5f into llvm:main Mar 8, 2024
6 checks passed
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request May 4, 2024
This patch adds repository checks to the release-binaries workflow jobs.
People were observing that the job was running on a schedule in their
forks. This only happens on old forks, but those probably exist in great
number given how prolific LLVM is. This is also good practice anyways,
on top of solving the direct problem of these jobs running with the cron
schedule on people's forks.

(cherry picked from commit 9f5be5f)
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request May 9, 2024
This patch adds repository checks to the release-binaries workflow jobs.
People were observing that the job was running on a schedule in their
forks. This only happens on old forks, but those probably exist in great
number given how prolific LLVM is. This is also good practice anyways,
on top of solving the direct problem of these jobs running with the cron
schedule on people's forks.

(cherry picked from commit 9f5be5f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants