8000 Do we need to run actions on forks? · Issue #7066 · python/typeshed · GitHub
[go: up one dir, main page]
Skip to content

Do we need to run actions on forks? #7066

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.

Sign up for GitHub

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

Jump to bottom
Closed
sobolevn opened this issue Jan 28, 2022 · 7 comments
Closed

Do we need to run actions on forks? #7066

sobolevn opened this issue Jan 28, 2022 · 7 comments

Comments

@sobolevn
Copy link
Member

Right now each my PR is tested twice: on python/typeshed@issue-branch and sobolevn/typeshed@issue-branch. Do we really want this?

Снимок экрана 2022-01-28 в 11 04 50

In case we want to switch it off, it is as simple as:

on:
  workflow_dispatch:
  push:
    branches: [master]
  pull_request:
    branches: [master]

Here: https://github.com/python/typeshed/blob/master/.github/workflows/tests.yml#L5
I can send a PR if needed 🙂
@hauntsaninja
Copy link
Collaborator

There have been times where I've found actions on my fork useful (although not so much these days). But I could also just open a draft PR to typeshed. Also you could turn off actions for your fork at https://github.com/sobolevn/typeshed/settings/actions So there's a lot of choices!

@sobolevn
Copy link
Member Author

In my point of view, disabling PRs on forks by default is a good thing!
Let's wait for other opinions 🙂

@Akuli
Copy link
Collaborator
Akuli commented Jan 28, 2022

I like this. If we decide to disable, and someone wants it back, they can just edit the yml file in their fork.

Also, I have never seen branches: [master] under pull_request before. It works without it too.

@sobolevn sobolevn mentioned this issue Jan 29, 2022
Closed
@hugovk
Copy link
Member
hugovk commented Jan 30, 2022

I'm personally strongly against this, editing the yml in their fork, and then having to remember to revert it before creating a PR, is an extra impediment to contributing.

Or I can submit a PR on my branch first to check it passes -> extra impediment.

Or I can develop on my master -> not a best practice, we should use feature branches, and I only have a single master -> extra impediment.

The fact it's tested on upstream and on the fork isn't such a big deal: the testing is split between two different accounts and therefore doesn't use up any extra quota/time from any single account.

CIs allow us to test on a much larger matrix of Python versions and operating systems, much more than is possible locally and sometimes faster too. I sometimes avoid contributing to projects with no/broken CI.

I believe the use of CIs are of huge benefit for software quality and development, especially when so easy nowadays; Travis CI really revolutionised this, remember how hard testing was before. We should rather encourage people to test on their own forks before creating PRs.

Fail fast: shorten the time to finding a failure, enable quicker loops of incremental development.

@Akuli Akuli closed this as completed Feb 5, 2022
@hugovk hugovk mentioned this issue Feb 23, 2022
Merged
@hugovk hugovk mentioned this issue May 30, 2022
Closed
@nightlark
Copy link

Most of the concerns are non-issues since there exists an alternative due to the workflow_dispatch trigger allowing you to manually to trigger CI runs on any branch in a fork. See below screen capture for how to access the feature.

ci-workflow-dispatch-trigger-typeshed

I'm personally strongly against this, editing the yml in their fork, and then having to remember to revert it before creating a PR, is an extra impediment to contributing.

Editing the yaml file is not necessary, unless your branch was created based off an old main/master branch that pre-dates the addition of the workflow_dispatch trigger. In which case you should probably rebase the branch or update it to be more in sync with main/master; in which case reverting would not be required.

Or I can submit a PR on my branch first to check it passes -> extra impediment.

Using the workflow_dispatch trigger doesn't require opening any PRs. It also doesn't require making any spammy commits.

Or I can develop on my master -> not a best practice, we should use feature branches, and I only have a single master -> extra impediment.

You can develop on a feature branch with any name you want, it will show up in the list of branches you can select when triggering the workflow.

I believe the use of CIs are of huge benefit for software quality and development, especially when so easy nowadays; Travis CI really revolutionised this, remember how hard testing was before. We should rather encourage people to test on their own forks before creating PRs.

Fail fast: shorten the time to finding a failure, enable quicker loops of incremental development.

The biggest downside to automatically running multiple tests is if you make multiple commits one after another making minor changes, then all of them will trigger a potentially large matrix of tests -- last I remember, each account has a concurrency limit of 20 running jobs across all personal repositories.

@hugovk
Copy link
Member
hugovk commented May 30, 2022

They're non-non-issues ;)

Sure, it's possible to trigger via workflow_dispatch, but it's still an extra impediment. Making the human do more work so the computer can do less.

As shown in your screencast, it can take 6 clicks and 15 seconds to trigger each and every build.

That's quite a bit of tedious manual labour.

Automated tests should be automatic :)

@AlexWaygood
Copy link
Member

Hi! I think we're pretty happy with our CI over here at typeshed. This issue has been closed for a while, and our CI situation has changed in the meantime because we were running out of CI minutes on a regular basis (#7883).

I'm sure there are further improvements we could make, but I'm not sure it's helpful to have a proxy debate here, on a closed issue, for a disagreement that's happening over on the bedevere issue tracker. If anybody wants to propose further changes to our CI, they're of course welcome to open a new issue or PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not run actions on forks
6 participants
@hugovk @nightlark @sobolevn @hauntsaninja @Akuli @AlexWaygood
0