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

[ci] add workflow_dispatch trigger #75336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Mar 25, 2023

The workflow_dispatch event allows manually triggering workflows either through the web GUI, the CLI or the API. See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch and https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow.

@umarcor umarcor requested a review from a team as a code owner March 25, 2023 23:38
@umarcor umarcor force-pushed the umarcor/ci/workflow_dispatch branch from 27d621f to 0d31cbe Compare March 26, 2023 02:13
@YuriSizov
Copy link
Contributor

Yes, but why? We never needed this, so there is no reason to add it. Only changes should trigger workflows, and if a run has failed we can already restart it.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

@YuriSizov the workflow_dispatch trigger needs to be in the default branch (https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow). Therefore, if contributors want to use the feature in their forks, they are currently forced to keep a master (default) branch rebased on top of the master here (upstream).
If this option is added here, contributors can maintain the master branch untouched and work on feature branches only.

Furthermore, this is related to #75338. There, the push and pull_request events are replaced with workflow_call. There is a single entrypoint triggered by changes, which then calls the reusable workflows. In that context, contributors can comment the push event of the pipeline, to prevent all workflows from being executed each time they push to a feature branch they working on. Then, through the workflow_dispatch, they can trigger the jobs they want to have tested only. When they are ready to open a PR, removing the comment of the push event in the pipeline will suffice.

@YuriSizov
Copy link
Contributor

If this option is added here, contributors can maintain the master branch untouched and work on feature branches only.

That still doesn't explain why this is needed. It serves no purpose for contributors either, as there is no practical reason to re-run any task. Except if you are working on CI specifically, perhaps. But that affects a tiny amount of contributors who can temporarily modify their master branch.

Furthermore, this is related to #75338.

It doesn't need a dedicated PR if this is somehow useful for #75338. It can remain a part of that for the time being.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

@YuriSizov, this is relevant for contributors that don't want their limited CI resources blocked when they rebase 2-4 feature branches after fetching the upstream.

Contributors can comment the push event in their feature branches to prevent all the workflows from running after each push. That's just one commit that can be cherry-picked to any feature branch that is being worked on. That allows to rebase as many branches as desired, without using CI.

Then, if they want to run a single workflow on some branch, they currently need to edit it to uncomment the push trigger. Or, they need to have different CI related commits in each feature branch.

Having the workflow_dispatch avoids having to do that. They can disable CI "by default" cherry-picking a single commit and then manually trigger the workflows they want when they want.

So, we can ask contributors to keep a custom default branch, or we can ask them to use multiple commits to selectively enable/disable CI in their feature branches. Alternatively, we can make their life easier by adding one keyword to the workflows, and let them focus on improving the engine and the documentation rather than dealing with CI.

Note that the use case explained here is the same as the one in #75338. The difference is that currently 7 files need to be modified in order to disable CI by default and #75338 will make it less verbose by allowing to just modify one file. However, the use cases that benefit from the workflow_dispatch are the same. Therefore, if this discussion is resolved as the feature not being needed, I will remove it from there.

@YuriSizov
Copy link
Contributor

Alternatively, we can make their life easier by adding one keyword to the workflows, and let them focus on improving the engine and the documentation rather than dealing with CI.

My issue here is not that adding one more trigger condition is some kind of burden, but that there is currently nobody who voiced a problem that you describe, nobody that came forward and explained that this is a workflow they use and that they need it. Well, I can assume that you have this issue, but that's a very small amount of data point to discuss if this is helpful to contributors in general or not.

If you have limited CI resources, you can just cancel some of the CI runs. It's the same kind of manual labor as you propose with this workflow, just in the opposite direction. But even then, 2-4 branches don't really make a dent in personal CI resources.

@umarcor umarcor force-pushed the umarcor/ci/workflow_dispatch branch from 0d31cbe to 682bbe9 Compare March 26, 2023 23:15
@umarcor umarcor marked this pull request as draft March 26, 2023 23:15
@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

FTR, I made this into a draft, since it's now based on #75338.

If you have limited CI resources, you can just cancel some of the CI runs. It's the same kind of manual labor as you propose with this workflow, just in the opposite direction.

A. Cancel 7 workflows each time a commit is pushed.
B. Do nothing and just trigger one workflow when needed.

Both solutions are manual, but I wouldn't say it's the same kind of labor. B is negligible effort, A is non-negligible at all.

But even then, 2-4 branches don't really make a dent in personal CI resources.

Having 13/20 concurrent jobs per commit, some of them taking 25, 40, 60 minutes to finish feels pretty relevant to me.

Well, I can assume that you have this issue, but that's a very small amount of data point to discuss if this is helpful to contributors in general or not.

Precisely, I voiced the problem, came forward and explained that it is a workflow I use and that I need it. Moreover, I explained that alternatives exist and which I can use and why they are not desirable. Still, I'm missing the data point to explain that this is not helpful for contributors, or even harmful to anyone. I cannot see even the maintenance cost of this feature.

@YuriSizov
Copy link
Contributor

Let me explain a bit more where I'm coming from. As I've said before, this is not about the maintenance cost. A general practice when contributing to this project is to identify a problem and prove its significance before doing any enhancements. While the buildsystem and the CI specifically are rather niche, I believe it still applies to them.

The problem with doing small arbitrary changes like this is that we don't have a good indication that many users need it, now or in future. If someone decides to work on this a few months or years later, all they'll find is that one user wanted it for their specific workflow and it was added because "Why not". So while doing future work on this, they'd wonder. Should it be preserved? Should it be ignored? Is this contributor alone in this, are they even still around?

If we have a clear understanding that this is a workflow that affects many contributors or is useful for many contributors, is something that they could use, then we can do add it. But at this moment this is just about a change that makes sense to you personally, based on a workflow that I'm not aware anyone else is using or needing. And I'll be honest, I'm evaluating all of the PRs you've opened in the last couple of days together. They paint to me a picture that you're starting contributing (again, I guess) and experiencing some early issues with the process. Of course you have an urge to fix them, and the changes make sense to you. But we need to be sure they make sense for everyone.

And so far I think it's not something that fits anyone else's workflow. But I might be wrong.

@umarcor umarcor force-pushed the umarcor/ci/workflow_dispatch branch 3 times, most recently from c916526 to 5a48dc0 Compare March 28, 2023 18:23
@umarcor umarcor force-pushed the umarcor/ci/workflow_dispatch branch from 5a48dc0 to cb04ff1 Compare April 5, 2023 12:16
@umarcor umarcor force-pushed the umarcor/ci/workflow_dispatch branch from cb04ff1 to 9d50fe2 Compare April 5, 2023 18:36
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

2 participants