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

Allow manually running GitHub Actions workflows and use consistent file extension #2215

Merged
merged 6 commits into from
Jan 20, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 7, 2022

Several changes:

  1. yaml -> yml, which is the prefered extension
  2. Specifies branch which should be used as a trigger. This is important because otherwise jobs can be runned twice: once for PR and once for push. This is a common and well-known problem

@sobolevn sobolevn requested a review from a team as a code owner January 7, 2022 23:37
@brettcannon
Copy link
Member

I'll give other people some time to review to see if they want to.

@hugovk
Copy link
Member

hugovk commented Jan 7, 2022

I don't like this branch restriction, because I prefer to check the CI passes for my own fork's feature branch first, before creating a PR.

That way I can iterate and only create the PR when it's ready, and don't need to create more noise in the main repo.

With a restriction, the options are:

  1. just create a PR and iterate here -> noise
  2. develop in my main branch -> no other feature branches
  3. make a temporary commit to disable this restriction, iterate in my fork, then remember to drop the commit before creating the PR

I think the double build thing isn't such a big problem for open source: normally the push build is done on the contributor's fork's CI, and the PR build is done on the upstream CI, so we won't be tying up the project's CI.

And for this repo, the CI is very quick: 1m19s + 30s.

If it really is a problem, a compromise is to allow something like "test-me-*": pytest-dev/pytest#9467

@sobolevn
Copy link
Member Author

sobolevn commented Jan 8, 2022

@hugovk good one!

If I need something similar, I would just open an intermediate PR in my own fork to main branch, while working on some feature-* branch. This way I can trigger the CI as it is intended to.

Later, you can close your own PR and re-create it from your feature-* branch targeting not your own main, but origin's main.

What do you think? 🙂

@hugovk
Copy link
Member

hugovk commented Jan 8, 2022

Yes, that is a fourth option, but I'm less inclined to do that as then I'll end up opening and closing hundreds of PRs.

I'll likely do option 2 (work in main) and/or 3 instead (temporarily disable then re-enable).


Generally speaking, I think contributors should be encouraged to easily test in their forks before creating PRs. Very often contributor guides recommend running tests locally, which is good, but can involve installing a lot of stuff. Sometimes they have tox, making it easy, but I might not have all Python version installed, and it can be slow.

But CIs are extremely useful and powerful, and often test on a much bigger matrix than I have access to: multiple operating systems, architectures, extra combinations. They're faster than my laptop. If a passing CI is considered a merge requirement, let us fail fast and find problems earlier in the contribution loop.

On the other hand, if the contributor isn't even interested in the CI, chances are they've not enabled it, and we're not even saving anything.


How much of a problem really is it to run the CI twice? Once in the contributor's fork and one in the upstream, separate accounts with separate limits/queues.

@sobolevn
Copy link
Member Author

sobolevn commented Jan 8, 2022

How much of a problem really is it to run the CI twice?

This is only a problem if:

  • The CI takes a significant amount of time
  • People who have access often push to origin feature-* branches

For example, it is important for CPython: https://github.com/python/cpython/blob/45d44b950f1dab0ef90d0a8f4fa75ffaae71500b/.github/workflows/build.yml#L10

But, I now think that this is not the case for python/peps 🤔

@hugovk
Copy link
Member

hugovk commented Jan 8, 2022

This is only a problem if:

Let's consider the main Cpython repo:

  • The CI takes a significant amount of time

Check.

  • People who have access often push to origin feature-* branches

Looks like this is not the case.

I see only three feature branches by two core developers:

A quick check of the first page of PRs: 6 PRs by 3 core developers, all from their own forks:

Even the miss-islington bot uses, erm, her/their/its(?) own fork for branches.


For example, it is important for CPython: python/cpython@45d44b9/.github/workflows/build.yml#L10

I believe it's not and is an impediment to contribution: I often temporarily disable that restriction before creating PRs. But I'm not going to take up this case now :)

But, I now think that this is not the case for python/peps 🤔

Agreed on both bullet points.

@sobolevn
Copy link
Member Author

sobolevn commented Jan 8, 2022

@hugovk 👍

I've removed branch name restriction, but added workflow_dispatch event, so the CI can be re-triggered manually by collaborators (I've missed it previously).

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AA-Turner
Copy link
Member

@sobolevn if adding workflow_dispatch to the other two, it may be worth doing so for the sphinx build process (deploy-gh-pages) - more as a just in case thing.

I've never known what the preferred extension is for YAML (this site says .yaml, custom & practice leads to .yml), but maybe good to be internally consistent and have all the workkflow files have the same suffix, as you've done!

A

@hugovk
Copy link
Member

hugovk commented Jan 8, 2022

Looking at the contents of the 4,764 sdists I've managed to download from the top 5,000 on PyPI:

  • 11,770 *.yml
  • 9,914 *.yaml

And:

  • 624 .github/workflows/*.yml
  • 38 .github/workflows/*.yaml

@AA-Turner
Copy link
Member

AA-Turner commented Jan 8, 2022

The statistician in me wants to ask about measurement error bias arising from sdists -- how many include the full repo vs just source, tests, docs etc.

But for a pretty inconsequential thing I probably shouldn't 😁

A

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Jan 12, 2022

FWIW, while I do find workflows running on my forks to be annoying, GH Actions can always be disabled with a couple clicks under your fork's repo settings, which is how I handle it and for contributors that use it on their forks (I personally run the pre-commit checks and both build systems locally myself), better than the workarounds.

Typically, we (Spyder projects and other things I maintain) limit it to only pushes and pull requests against designated branches, to avoid duplication, and at least for the Spyder project and others I work on, submitting a draft PR early to run the CIs isn't really seen as too noisy, at least more than duplicate check runs.

We typically use:

on:
  push:
    branches:
    - master
    - main
    - '*.x'
    - 'staging*'
  pull_request:
    branches:
    - master
    - main
    - '*.x'
    - 'staging*'

This runs against our main branch, release branches, and any special "staging" branches, which can be pushed to in order to test changes before merging to a production branch (which can be used by contributors on their own repos here).

I've never known what the preferred extension is for YAML (this site says .yaml, custom & practice leads to .yml), but maybe good to be internally consistent and have all the workkflow files have the same suffix, as you've done!

FYI, that site is the official YAML spec, which as you mention authoritatively states that the extension should be .yaml, not .yml. As such, while consistency is more important overall, given the existing .yaml extension is correct per the standard, I would not advise renaming them existing files to not follow it.

@AA-Turner
Copy link
Member

To add a vote in favour of the PR as it currently stands -- I'm with Hugo and find CI on my fork really useful.

RE special branch names -- perhaps a reasonable solution when CI takes ages or a lot of resource, but it is (IMO) not an amazing precendent, as projects will inevitably use different magic phrases, leading to frustration. The key point here for PEPs is that CI is really quick, and hopefully at some point soon will be even quicker as there'll only be one build job, not two.

RE suffixes -- I don't mind!

A

@AA-Turner
Copy link
Member

@sobolevn please could you resolve the merge conflicts?

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the preferred extension per the official spec is .yaml, not .yml, its more important that they be consistent one way or the other and its not worth bikeshedding over. As for the trigger config, makes sense to me for this repo 👍

@CAM-Gerlach
Copy link
Member

Looks like the merge conflicts are fixed and we all approve, so going ahead with this. Thanks @sobolevn !

@CAM-Gerlach CAM-Gerlach changed the title Improve GitHub Actions definition Allow manually running GitHub Actions workflows and use consistent file extension Jan 20, 2022
@CAM-Gerlach CAM-Gerlach merged commit d9e63cc into python:main Jan 20, 2022
@merwok
Copy link
Member

merwok commented Jan 20, 2022

I would like to repeat that «consistency» for its own sake, and especially about such a tiny thing as a file extension, is not a sufficient reason to do changes in Python projects (CPython for sure, for related projects it depends). Increasing consistency (often meaning uniformity or alignment of details) is not a goal in itself; it should be: fixing an inconsistency that does trip up people, or that makes development tools harder to use (if they have glob patterns for example), or that creates another kind of trouble or impediment for people reading, using or changing code or other files.

There is much more value in spending time to fixi reported issues than observing inconsequential misalignment of details and «fixing» them.

(I am not saying that attention to details is bad, or that new code/docs should not strive to be generally consistent internally and with existing conventions, or that there doesn‘t exist sloppiness that does cause issues down the road. It’s about balance in judgment and knowing where to invest time. And thank you for all your efforts and time spent on the project!)

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.

7 participants