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: Do not run actions on draft PRs #722

Closed
wants to merge 3 commits into from

Conversation

nikcio
Copy link
Contributor

@nikcio nikcio commented Oct 25, 2022

GitHub actions should not run on draft PR's this wastes runners for other open PR's.

See .github/workflows/Generate-TestWorkflows.ps1 to see what changed.

@nikcio nikcio marked this pull request as ready for review October 25, 2022 13:21
@NightOwl888
Copy link
Contributor

I am torn about this. On one hand, having the ability to stop the tests from running is good for the reasons stated. However, it also means you cannot open a PR as a draft (indicating it isn't ready for pull) without also skipping the tests.

This means to run the tests thoroughly on all operating systems without doing it in a draft PR, a committer will either have to submit a regular PR (even though it isn't ready for pull), have all of those operating systems available locally to test with, or set up an Azure DevOps organization, repo, and build pipeline. Unfortunately, Microsoft has put a manual approval process in place to prevent abuse, so it can take several days after setting up Azure DevOps until you can run a pipeline.

But maybe we can accommodate this without excluding all draft PRs from tests. I was going to suggest setting up an action:run-draft-pr-tests label that can be added to a PR to run it, but I am pretty sure that non-Apache committers cannot add labels (or can they?) We could also have a single file that when edited controls whether or not the draft PR tests run, but that doesn't seem that elegant. Thoughts on how to conditionally run tests on a draft PR (that default to not run)?

NOTE: Apache's agent availability is down this month (presumably because of Hacktoberfest). Under most normal days we can get 80 or 90 parallel agents to run, and the whole test run takes less than 10 minutes. Unfortunately, their agent availability varies greatly, and the same test run can take 9 or 10 hours if there are only 1 or 2 parallel agents. Azure DevOps is much more stable - a build/test cycle almost always takes exactly 40 minutes with a maximum of 10 parallel agents.

Unfortunately, Apache won't open up the permissions to integrate with Azure DevOps so options are limited with using it at scale.

@nikcio
Copy link
Contributor Author

nikcio commented Oct 25, 2022

I've looked a bit around and it might be possible to get a little of both worlds. Using the setting explained here:

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

We should be able to

  1. Prevent first time contributors from running the workflows without approval or
  2. Prevent "outsiders" from having workflows run automatically

The 2nd option gives a bit more power but also requires more micromanaging.

Furthermore it's possible to trigger a workflow when labeled so we could add a specific label to run tests in draft PRs and then extend the if statement to include an "or label x is present".

It is not possible to add a label to a PR when being an "outsider" like me. But this might also help prevent possible abuse and should be quite quick for someone with access to add a label should it be required.

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

@NightOwl888
Copy link
Contributor

Issue Templates

I reported a bug recently to Lucene, and their bug report template automatically assigns a label to an issue (even though I am not a Lucene committer).

I have been meaning to set up issue templates for Lucene.NET at some point, especially because bug reports rarely provide the necessary information to reproduce the problem.

  1. Which target framework?
  2. Which operating system?
  3. Which version of Lucene.NET?
  4. Provide a minimal working example of the code required to reproduce (either in a console app or a test). That is - there should be enough code to create an index with dummy data and a similar configuration of various pluggable components (or their defaults) in order to perform the operation that is causing the issue.
  5. What is the observed behavior?
  6. What is the expected behavior?

The dotnet runtime repo has a great example of what types of things are possible. Looks like it may also be possible to provide external links. So, we could potentially redirect people with user questions to either StackOverflow or the user mailing list.

That sounds like a new issue that will require some research and discussion to do - no need to submit that to this PR.

Pull Request Templates

Anyway, after some preliminary research, it turns out that templates are also supported for PRs and it appears (though not confirmed) that adding a template to produce a PR with a specific set of labels is also possible using the YAML form schema. The idea is we would create different categories such as "Resolve Issue PR", "Draft PR", and "Draft PR with Tests", the latter of which would include a label that would fire the tests. Or maybe we could just make a "Draft PR" with a form element that allows the user to select to run tests or not, this will require some research and experimentation.

FYI: Do note that we don't have access to the GitHub Settings page, however Apache has automated setup of a range of settings through various means, most prominently the .asf.yaml file. Of course, if a feature isn't currently supported, it can be discussed with INFRA and even contributed by those willing to learn how to do it and navigate the obstacles to make the contribution.

However, since issue and PR templates are entirely configured using files in the repo, this is something we can set up ourselves - even without being an Apache committer.

@NightOwl888
Copy link
Contributor

I've looked a bit around and it might be possible to get a little of both worlds. Using the setting explained here:

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#controlling-changes-from-forks-to-workflows-in-public-repositories

We should be able to

  1. Prevent first time contributors from running the workflows without approval or
  2. Prevent "outsiders" from having workflows run automatically

Apache has recently changed the default setup so first-time contributors require manual approval before the tests run. It looks like those settings are configurable under the Branch Protection settings of the .asf.yaml file.

@paulirwin
Copy link
Contributor

Of note, being able to run actions on draft PRs is of use to me in #996 currently as I work through fixing issues with it. Given @NightOwl888's most recent comment here about first-time contributors to address that issue, should we close this PR?

@nikcio
Copy link
Contributor Author

nikcio commented Oct 25, 2024

@paulirwin i think it's fine to close this PR as it solves an issue that isn't really that relevant in the state the project is in right now I'll close to avoid wasting more affort on this 😁

@nikcio nikcio closed this Oct 25, 2024
@paulirwin
Copy link
Contributor

It was not wasted effort! Often, dead ends help inform the right solution. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants