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

Cron GitHub Actions workflows execute on forks #10235

Closed
mfisher87 opened this issue Jul 6, 2024 · 17 comments · Fixed by #10238
Closed

Cron GitHub Actions workflows execute on forks #10235

mfisher87 opened this issue Jul 6, 2024 · 17 comments · Fixed by #10238
Assignees
Milestone

Comments

@mfisher87
Copy link

mfisher87 commented Jul 6, 2024

Bug description

Cron workflows are running on forks! It's a bit spammy and wasteful.

image

I went to submit the fix as a PR but saw I needed to sign a CLA, and I don't have time for it :(

Feel free to copy from my PR on my fork, as I Just copied code from another workflow in this repo anyway 😆 mfisher87#1

Steps to reproduce

Fork this repository.

Expected behavior

No actions run when I'm not actively working on / pushing to my fork

Actual behavior

Actions run and I get notifications and energy is wasted

Your environment

No response

Quarto check output

N/A

@mfisher87 mfisher87 added the bug Something isn't working label Jul 6, 2024
@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

Quarto has no control over forks. You are the owner of a fork.
You allowed the GitHub Actions to run on your fork.
It's up to you to disable it (go to your repository/fork setting > Actions and disables the actions).

@mcanouil mcanouil closed this as not planned Won't fix, can't repro, duplicate, stale Jul 6, 2024
@mcanouil mcanouil added support a request for support and removed bug Something isn't working labels Jul 6, 2024
@mfisher87
Copy link
Author

mfisher87 commented Jul 6, 2024

@mcanouil I want to use GitHub actions, just not on a cron basis. Quarto does have control over forks, and it exercises it as below:

if: github.event_name != 'schedule' || (github.event_name == 'schedule' && github.repository == 'quarto-dev/quarto-cli')

However, it only exercises this technique on one workflow instead of all workflows. Can this please be applied to all workflows that use cron?

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

Again, user enables or not the workflows to run.
That's not up to the upstream repository.
So no Quarto has absolutely no control about what is happening in forks.

You can disable the workflow on your fork if you don't want one in particular and want the others for some reasons.

What you are showing in the workflow is completely unrelated to the current matter.
Specifying an if key that checks the name of the repository is pretty much rare and in this case I don't know what's the purpose of this condition.

@mfisher87
Copy link
Author

mfisher87 commented Jul 6, 2024

Can you explain how it's unrelated? That conditional is enabling the workflow to run if the event triggering it is not a "schedule" event (i.e. cron) OR the event is "schedule" and is running on the current repository. The 2nd half of the OR would evaluate to false for a cron event on a fork.

@mfisher87
Copy link
Author

Specifying an if key that checks the name of the repository is pretty much rare and in this case I don't know what's the purpose of this condition.

Thanks for updating your post with further explanation. The purpose of the condition is exactly the solve the problem I am opening this issue to report! When the repository name is not "quarto-dev/quarto-cli", the repository is a fork. Running cron workflows on all Quarto's forks, and please keep in mind there are 293 at this point, is expending a non-trivial amount of energy. It is responsible, even if rare, to avoid it, IMO. But given that this repository is already doing it in one place, I wouldn't call it "rare" myself :)

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

By default, actions are disabled on fork (unless GitHub changed that recently)
If a user decides to enable actions to run on forks, then that's up to that user to enable/disable workflows.
I don't see why Quarto would be responsible for this.

The workflows are there to tests many things for Quarto to work.
What you suggest would prevent user to make changes on their fork and tests their changes.

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

Take for example: https://github.com/mfisher87/quarto-feedstock

Should the upstream repository also hardcode its repository name in the workflows to forbid users forking the repository to run them?

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

You have 100% control over what is happening in your fork.
I suggest you go to the action tab, go to the workflows you don't need/want in your fork and disable their execution.
image

Or disable all of the workflows if you don't need them at all

image

@mfisher87
Copy link
Author

mfisher87 commented Jul 6, 2024

I don't think you're understanding me. I do not want to disable GitHub Actions for my fork. I do not want to disable any workflows on my fork. I want them to not be triggered by cron.

There are 293 forks of Quarto running cron workflows every day, and I am pointing out that this is very wasteful and that you can easily prevent that without any of those users needing to take action. You've already done it on one workflow, so you can do it again by copying and pasting that.

I don't think I've had this much difficulty communicating with this project before, and I apologize if I'm being unclear or misunderstanding you in some way, but at this point I feel we're talking past each other about different things. @cderv @dragonstyle would you be able to help clear up this misunderstanding?

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

Ok, I understand your issue.

Note that your statement about 293 running actions is wrong.
You can easily checks that. Out of the first ten forks I looked, only one had actions and nothing ran recently.

@mfisher87
Copy link
Author

I will absolutely concede that some of the 293 forks have disabled GitHub Actions, but that's not the issue I'm trying to address here. I'm not here to argue.

@mcanouil mcanouil added maintenance and removed support a request for support labels Jul 6, 2024
@mcanouil mcanouil self-assigned this Jul 6, 2024
@mcanouil mcanouil reopened this Jul 6, 2024
@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

Only a few enabled the workflows on their fork (not the other way around as the default is not to activate actions on fork exactly for the reason you exposed, i.e., activating actions on forks is a waste of resources most of the time).

Anyhow, I made a PR, but in the end I am not the one that is going to decide to merge it or not.

@mfisher87
Copy link
Author

Thank you for being open to this request and for opening a PR!

At the risk of "beating a dead horse", and I apologize in advance if this comes across that way, but I understand that the default for forks is that Actions are disabled, and I agree that's a good thing! However, when I fork something, it's because I want to contribute, and when I want to contribute, I want to make sure my tests pass and stuff before I open a PR on the upstream repo, because I don't want to burden the maintainers with stuff that doesn't meet their quality standards. I can empathize with needing a good signal/noise ratio, as I maintain some stuff too! So the first thing I do when I fork, and I suspect many contributors would do, is enable Actions and get to work. None of this is meant as a criticism, I'm just trying to share my pattern of work.

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 6, 2024

FYI, In the case of Quarto (and many other softwares), you don't need CI/CD.
You can follow the readme which describes how to run tests: https://github.com/quarto-dev/quarto-cli?tab=readme-ov-file#running-tests

(Running tests locally before making a PR is by far less wasteful on resources and with a way lower carbon footprint than running CI/CD on a fork that is going to be triggered also on the PR)

@mcanouil mcanouil added this to the v1.6 milestone Jul 6, 2024
@mfisher87
Copy link
Author

(Running tests locally before making a PR is by far less wasteful on resources and with a way lower carbon footprint than running CI/CD on a fork that is going to be triggered also on the PR)

I agree! I personally like to use pre-commit or pre-push hooks on my projects so I don't have to remember it, and run CI checks on PRs (I don't want to add a variable and start talking about CD 😆). But I do often trust an unfamiliar project's CI configuration to fill in for my lack of knowledge and help me know when I missed something important :)

@mcanouil
Copy link
Collaborator

mcanouil commented Jul 8, 2024

@mfisher87 changes have been merged.
If you pull the changes in your fork, schedule trigger should no longer work.

@mfisher87
Copy link
Author

Thanks all for your help addressing this! 🌳 🌲 🌴 ❤️

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

Successfully merging a pull request may close this issue.

2 participants