BUG: Non-standard workflow/behavior

[larsoner]

It seems like the suggested action should have actions/checkout as most do like:

jobs:
  changelog_checker:
    name: Check if towncrier change log entry is correct
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v4
    - uses: scientific-python/[email protected]
      env:
      ...

This seems like a much more standard workflow than the action doing some magic with the base repo to fetch the pyproject.toml. Using a base repo file has at least a couple disadvantages that I just ran into because pyproject.toml from upstream/main is always used:

1. When adding the checker (as I just did) alongside towncrier functionality at all, the PR will fail until all the towncrier config stuff lands in main.
2. Tweaks to sections etc. cannot be made in a PR and still have the bot pass. Let's say I want to rename enhancement to newfeature -- in a PR I can't do this and have the bot pass because it will always pull from upstream/main. Yes you can skip the check with labels but that's not as nice as having everything work together.

I don't see any big advantages of using the baserepo stuff here, can we switch to actions/checkout instead?

[pllim]

I don't remember the full history now. Perhaps the setup was ported from the old change log bot that existed before Actions was a thing. It also existed before GitHub had safeguards in place to prevent people abusing CI in malicious PRs. I think always using upstream/main was a feature, not a bug. I wanted PR checks to follow upstream/main rules consistently; i.e., PR authors cannot change the check to their liking.

@bsipocz or @Cadair , any opinions here?

[Cadair]

I think the Giles check uses the head branch of the PR to achieve the desired behaviour of using the "merged" config.

I suppose there are probably other ways on actions to do that?

[larsoner]

I think the Giles check uses the head branch of the PR to achieve the desired behaviour of using the "merged" config.

Having used other actions for a while, it seems like the most standard behavior is to use actions/checkout, because it

1. Behaves nicely by default (using merged code for PRs)
2. Is configurable beyond the default (e.g., you can use the head branch if you want non-merged behavior), etc.
3. Does nice magical stuff like caching
4. Will be updated and well maintained

That plus using setup-python, which also caches, with this becoming a composite action seems like it would be potentially better and faster than rolling a custom dockerfile as is done now. End users could put this action in line with a series of other actions (like the pre-commit one, which is so very simple!) and they will go very quickly. Using a composite action could in theory take care of #1 (comment).

And to add to the above, in addition to it being surprising that it uses the root pyproject.toml for config, it's surprising to me that options for this action are set in pyproject.toml at all. Most actions use a with: to set this stuff. It seems like it would make the most sense to put all of these settings in the action itself, like:

- uses: actions/checkout@v4
- uses: actions/setup-python@v5
- uses: scientific-python/action-towncrier-changelog@v1
  with:
    verify-pr-number: true
    changelog-skip-label: no-changelog-entry-needed

So concretely I guess my set of proposals for consideration would be:

1. Outsource the checkout and setup-python-type stuff to standard steps (just like pre-commit's action does)
2. Make this a composite action
3. Release as v2 and move to standard v-based tagging (e.g., v2.0 gets released also as v2, but then v2 gets force-pushed over when v2.1 gets released for example)
4. Move all action options to the with: section of the action itself rather than relying on pyproject.toml. (If this is too painful, we could at least add support for falling back to pyproject.toml if the with: options are not supplied.)

I think it would make this action more standard, easier to use, and faster for at least some workflows. For MNE-Python for example, we'd just piggyback this uses in an existing style run that already checks out code and sets up python, so it'll only an extra few seconds to run this action instead of an extra minute.

[larsoner]

I wanted PR checks to follow upstream/main rules consistently; i.e., PR authors cannot change the check to their liking.

Do people actually do this? 😱 In years of maintaining multiple repos I've never actually seen someone try to mess with a CI config to get their PR green at least...

[pllim]

Do people actually do this?

Not... yet. But we have hundreds of contributors and I cannot oversee every PR. Not all the maintainers are savvy with Actions or all the policies. Some see a ✔️ and trusts it completely.

[larsoner]

Not... yet. But we have hundreds of contributors and I cannot oversee every PR. Not all the maintainers are savvy with Actions or all the policies. Some see a ✔️ and trusts it completely.

Sure but I think the same argument could be made for all workflows that do use actions/checkout etc., though -- which is the vast majority of them. Contributors can in principle mess with all sorts of things in actions if they want to, and you need reviewers to catch it. But given how rarely people mess with CI/testing infrastructure in practice, I don't find this a very compelling argument for why this particular action should deviate from what seems to be standard GHA coding/implementation practices.

Thoughts on the other ideas outlined in #10 (comment) ? For example point (4) above is how I'd prefer to take care of making things optional in #9 (comment) rather than messing with pyproject.toml. Basically if it is worth overhauling the action in the way I specify above I'd rather do that first so that I can implement new stuff in the standard ways from the start, rather than doing them in a less common way and having to change it later.

[bsipocz]

I, for one, agree with @larsoner here to follow the more standard GHA approaches (I might have come across contributors messing with the tests to get the green check, but can't recall anyone changing the ci config directly). But I don't have much stake in this particular action, and don't directly use it in my projects.

(Side note: using codeowners, or some extra protection for the .github directory may be a safer approach to safeguard against unsolicited CI changes)

[pllim]

but I think the same argument could be made for all workflows

True. For CI, there isn't a choice because it needs to pull in the patched code to test.

follow the more standard GHA approaches

If you want to put in the work, I guess it is fine given I do indeed never saw it abused the way I mentioned. If we see it some day, we can always consider reverting or something else. I can see how the benefits of refactoring might outweigh the risks here.

Thanks, all!

[pllim]

@larsoner , would your #9 fix this?

[larsoner]

No, it doesn't do anything about the non-standard (for GHA) configuration / settings etc.

[adrinjalali]

We also just had the same issue in scikit-learn/scikit-learn#29705 since it adds a section to pyproject.toml file. It seems very odd to me that the action wouldn't checkout the PR branch to get the config file. There are cases where we make sure the action gets info from the main repo for security reasons, but checking the changelog is not one of those high risk areas where it'd matter.