ci: fix pytest with typo

[bobleesj]

This PR fixes pytest

[bobleesj]

@sbillinge ready for review

[bobleesj]

since we don't have matrix CI, adding this to push just in case we make a direct commit to main by mistake.

[sbillinge]

I am a little confused why we want this. Won't this trigger this workflow to run every time we merge to main? Which is undesirable behvaior? I guess you want to stop a PR that is failing this CI to be merged to main, is that the intention?

We should think about this. It may be too "fussy" and create overhead and frustration. What if, as a human being, I want to merge something that is failing CI for some reason. Will this prevent me from doing that?

[sbillinge]

Thanks for this fix. Please see my comment. I am worried that we are doing too much to protect people from themselves. If they follow protocols, this won't happen. @Sparks29032 threw me under the bus a bit there (thanks @Sparks29032 :) but there was presumably some reason why we did it.

[sbillinge]

I am a little confused why we want this. Won't this trigger this workflow to run every time we merge to main? Which is undesirable behvaior? I guess you want to stop a PR that is failing this CI to be merged to main, is that the intention?

We should think about this. It may be too "fussy" and create overhead and frustration. What if, as a human being, I want to merge something that is failing CI for some reason. Will this prevent me from doing that?

[bobleesj]

I guess you want to stop a PR that is failing this CI

The intension that, for some reason one of us makes a commit directly to the main, we want to ensure the test still runs.

we only had "pre-commit" running so it was a green mark but it didn't run the tests.

[bobleesj]

In principle, no one really should make a direct commit but it's more like a "safety" mecheanism. Of course, one way is to not allowing anyone to make a commit directly to main.

[Sparks29032]

Running on each push seems like a lot, especially as a safety mechanism to catch something that should not be done in general. Theoretically, only the repo maintainers have direct push access and pre-commit should already act as a warning (oops on my part for pushing directly without following the full protocol).

If we really do want to add this, we should probably make it a conditional check to run only if a PR was not created.

https://github.com/orgs/community/discussions/26276 (probably do not want this as we want to run on other dev branches as well)
https://github.com/marketplace/actions/skip-duplicate-actions (could be used to do this conditional check)

[sbillinge]

I am worried that this is overkill. If we want this security we can set up the permissions on the repo to prevent direct merges to the default branch, right? Unfortunately it is a bit clunky to set it up or I would do it on all the repos, but can we instead use the ruleset in the repo itself to disallow a direct merge to main, but somehow, in skpkg, provide some tools that make this easier to set up, something like that. In any case, let's remove it from this PR so it can be merged, and we can discuss it on another issue if the discussion is not resolved?

[bobleesj]

ruleset in the repo itself to disallow a direct merge to main

I think we can set this up at the "org" level so should an easy fix. We used this org level feature for PyPI token, etc.

[sbillinge]

ruleset in the repo itself to disallow a direct merge to main

I think we can set this up at the "org" level so should an easy fix. We used this org level feature for PyPI token, etc.

ooh, that is good to know. Let's work on that because I would like to have an org level ruleset that is minimal but not zero....please let me know if you figure out how to do that.

[bobleesj]

https://github.com/organizations/scikit-package/settings/rules

Should be here @sbillinge