search

quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.82k stars 309 forks source link

Cron GitHub Actions workflows execute on forks #10235

Closed mfisher87 closed 2 months ago

mfisher87 commented 3 months ago

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 :laughing: https://github.com/mfisher87/quarto-cli/pull/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

mcanouil commented 3 months ago

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).

mfisher87 commented 3 months ago

@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:

https://github.com/quarto-dev/quarto-cli/blob/b7150a0d767cee7d0aac1796bfe57847b3c65160/.github/workflows/create-release.yml#L32

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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)

mfisher87 commented 3 months ago

(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 :laughing:). 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 commented 2 months ago

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

mfisher87 commented 2 months ago

Thanks all for your help addressing this! :deciduous_tree: :evergreen_tree: :palm_tree: :heart: