BUG: Ensure git is properly configured for code coverage action

[trallard]

After reviewing the logs for the failed actions and the python-coverage action, I noticed that the underlying problem is that git configurations like user.name for the github-actions[bot] are not set automatically.

The action passes these as environment variables when committing to the coverage branch. Still, they are not picked up in the workflow (so I might perform a bit more checking and possibly open an issue upstream).

I checked locally before pushing to the branch, and the 403 error is cleared after setting this configuration, so this will resolve our failing workflow. And this should supersede the WIP #2226 since that reverts to the action through workflow_run which is recommended against due to security reasons (which is why this was changed in the first place).

[gabalafou]

I realize this is a draft but I thought it might be helpful for me to weigh in now.

I don't think this current draft (i.e., setting git config) is going to fix anything.

I dug through the PCC action's source code. It's clear that the action does not support the workflow_call event.

Take a look at the find_activity() function. If you grep the codebase and follow how that function is used, it becomes clear that our configuration will not work. That's because the workflow_call event gets mapped to "process_pr", but processing the PR is what is supposed to happen in the first half of our two coverage jobs (the one in CI.yml), not the second half (in coverage.yml) which is supposed to "post_comment".

We should probably open an issue on their repo. I searched their issues for "workflow_call". Couldn't find anything. @trallard, I think it would be best if you create the issue because you can make the case from a CI security point of view better than I could.

Part of what makes this bug tricky is that there are several different bugs all at once, which you can see a bit more clearly in a PR I opened on my fork:

1. Persist credentials (more on this below)
2. event_name not event.name (pull_request event does not have name property)
3. Incorrect variable interpolation in GET /repos/"${REPO}"/actions/runs/"${WORKFLOW_RUN_ID}"

For that last one, there's no point in correcting the variable interpolation. The job step simply isn't being used, so I deleted it in that PR. When I looked into its history, it looks like that "get-run" step was added for a subsequent step that was then later deleted (step name: "Check if the trigger was a PR event").

Now, about persisting credentials—I am almost certain that we need to persist credentials in CI.yml for the "check coverage" job.

You can see this fairly clearly in the action logs for my fork. I made two different commits to the main branch. In one, I set persist-credentials to true, and the job passed. In the other I set it to false, and the job failed:

• ✅ check coverage job passes, persist-credentials: true
• ❌ check coverage job fails, persist-credentials: false

Reading the action source code also supports this hypothesis. It tries to git push (via subprocess) rather than use the GitHub API. I think the only way that a git push will work in the context of a workflow is if credentials are persisted after checking out the repo.

Here's the sequence of relevant function calls:

1. main.py: save_coverage_data_files() calls storage.commit_operations()
2. storage.py: commit_operations() calls git.push()
3. subprocess.py: git.push gets handled via Git.__getattr__ which ultimately goes to subprocess.run()

Hope that helps!

[trallard]

I am going to close this PR as I currently do not have the capacity right now to debug, iterate, and, more likely than not, follow up and contribute upstream.
And we have a lot other items in our backlog that I believe would be a better investment of my time right now (like the FA svg/webfont simplification which implies changes to some components and have to put on pause to follow on the back and forth over the various PRs related to this action, or getting the next PST release out, or working on the project's governance/maintenance tasks).

Bottom line, the effort to maintain the coverage comment action has significantly surpassed the value of having such an action in our repository.

• The main value proposition of this action is to add a comment to the PR with the coverage change (if any). However, when we added this action to our CI, we discussed that it was more of a nice-to-have, and we would reconsider keeping it based on the effort and usefulness it would provide. I believe we have put in a lot of effort into maintaining this action compared to what we get in return:
  • We monitor test coverage already and have pretty good summaries in our GitHub actions, so having a summary comment in the PR is not a deal breaker. (See, for example, https://github.com/pydata/pydata-sphinx-theme/actions/runs/15924099451, we get a comprehensive summary and information regarding the changes in coverage.)
  • This action has been fiddly since the very beginning, and we have had to make iterative changes to get it working (multiple times).
  • Pushing the coverage files to a separate branch is also nice to have, but not essential in any way. I never check these, as I use the summary in the actions, but I'm not sure if others have found this useful.
• To get this action working again, there are several caveats:
  • If we want to avoid persisting credentials (which I think we should), we'd need to reconfigure git and reauthenticate (which is what I was planning to try and do in this PR) to enable commit/push.
  • We already had a workaround to prevent the workflow from being called via workflow_run; hence, we are not using the action as is in their documentation, but that also requires debugging and patching.
  • The alternatives to the above would be one of the following:
    1. Follow the action's documentation (this means persisting credentials and using a less secure call, such as workflow_run¹²). I would rather not go down this route, as we have recently undertaken a significant amount of work to make our CI/CD pipelines more robust, streamlined, and secure. This would effectively be a regression in security. The marginal gain of having a comment in PRs seems significantly outweighed by keeping our workflows and repository secure.
    2. The alternative would be to suggest and likely contribute improvements to enable more secure usage of the action upstream. Going through the codebase, it is clear this would imply a significant amount of rework on the codebase (the maintainers might not want to do it, or not have the capacity, so it might come back to us to contribute the changes). This would benefit all action users, but I am currently unable to commit to such an effort due to other responsibilities.
    3. Absorb some of the tasks currently performed by the action (for example, posting the comment and pushing the coverage files) so we do not rely on a third-party action. But that means we also absorb the maintenance of this.

All in all, I suggest removing the coverage comment action from our CI altogether. It wouldn't significantly impact our workflow or that of our contributors (we'd have a fully working CI/CD, so that'd be better).
However, if anyone else feels differently and has both the availability and interest in pursuing any of the alternatives, that could still be okay.
Pinging @gabalafou and @drammock for thoughts before I close the PR.

Footnotes

1. These are dangerous because they run in the context of the target repository rather than the fork repository, while also being typically triggerable by the latter. This can lead to attacker-controlled code execution or unexpected action runs with context controlled by a malicious fork. ↩

2. Vulnerable GitHub actions workflows part 1: privilege escalation inside your CI/CD pipeline ↩

[gabalafou]

Totally agree.

You make a strong case that the value the action provides outweighs its cost (whether in security or maintenance.

Plus one thing you didn't mention
Edit: sorry, you did mention that, so to re-iterate what you already wrote, we already put coverage information in the workflow run summary page. Looks like this:

I'm fairly certain that that coverage table is posted from this line in the CI.yml file, which does not depend on the Python code coverage comment action.

pydata-sphinx-theme/.github/workflows/CI.yml

Line 147 in 51af2a2

python -Im coverage report --format=markdown >> $GITHUB_STEP_SUMMARY

So another argument for removing the third-party action is that we are still generating coverage information, even for pushes to main. What we lose with removing the action is:

• coverage comments on PRs
• coverage status badges via the python-coverage-comment-action-data branch, which I don't think we're even using (is there anything else that branch provides that isn't already provided, for example, by the coverage summary that we post ourselves to the workflow run?)

And I agree that doesn't seem to make the action worth it.

[trallard]

Correct that summary in GitHub actions is generated when we run the pytest tests.

Aside from pushing the coverage files and adding the comments to PRs there is nothing else we are getting from the third party action.

[drammock]

no arguments here; closing. I assume a PR to purge the action from the CI config will follow.