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

Client error occurred. status: 422 Unprocessable Entity Couldn't find a repository matching this job. #1721

Open
jrfnl opened this issue Jul 11, 2023 · 59 comments

Comments

@jrfnl
Copy link

jrfnl commented Jul 11, 2023

Similar to issue #1710 (which is still open, but seems resolved and involves coveralls-python).

I've started seeing the same as of today with GH Actions builds involving php-coveralls. The builds still worked find yesterday and no changes have been made to the workflows and PHP Coveralls have not released any new versions between yesterday and today.

Relevant part of the workflow:

      - name: Install Coveralls
        if: ${{ success() }}
        run: composer global require php-coveralls/php-coveralls:"^2.5.3" --no-interaction

      - name: Upload coverage results to Coveralls
        if: ${{ success() }}
        env:
          COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_TOKEN }}
          COVERALLS_PARALLEL: true
          COVERALLS_FLAG_NAME: php-${{ matrix.php }}-phpcs-${{ matrix.phpcs_version }}
        run: php-coveralls -v -x build/logs/clover.xml

  coveralls-finish:
    needs: coverage
    if: always() && needs.coverage.result == 'success'

    runs-on: ubuntu-latest

    steps:
      - name: Coveralls Finished
        uses: coverallsapp/github-action@v2
        with:
          github-token: ${{ secrets.COVERALLS_TOKEN }}
          parallel-finished: true

In the GH Actions log:

Read environment variables
Dump submitting json file: /home/runner/work/PHPCSUtils/PHPCSUtils/build/logs/coveralls-upload.json
File size: 494.11 kB
Submitting to https://coveralls.io/api/v1/jobs
Client error occurred. status: 422 Unprocessable Entity
Couldn't find a repository matching this job.
elapsed time: 0.776 sec memory: 8.00 MB

Link to the workflow file: https://github.com/PHPCSStandards/PHPCSUtils/blob/aa23fbf1cbdfa15ac381eec08169b8cb8dea1e49/.github/workflows/test.yml#L258-L385
Link to the GH Actions transcript for a failing build: https://github.com/PHPCSStandards/PHPCSUtils/actions/runs/5517110692/jobs/10074460474
Link to the repo on Coveralls: https://coveralls.io/github/PHPCSStandards/PHPCSUtils

@afinetooth
Copy link
Collaborator

afinetooth commented Jul 11, 2023

@jrfnl it looks like you're passing github for the service_name parameter, which is reserved by the Coveralls Github Action, which I see you are using in your parallel-finished step. However, what the Coveralls Github Token expects for the github-token value is ${{ secrets.GITHUB_TOKEN }}, rather than ${{ secrets.COVERALLS_TOKEN }}.

I know you said nothing changed with the workflow, but can you try swapping that value for github-token and letting me know if it changes your results?

@jrfnl
Copy link
Author

jrfnl commented Jul 12, 2023

@afinetooth Thanks for the quick response! Checked and it looks like that does fix it (for now).

Open (draft) PR: PHPCSStandards/PHPCSUtils#490
Workflow transscript: https://github.com/PHPCSStandards/PHPCSUtils/actions/runs/5528204578/jobs/10084780141#step:16:60
Received build on Coveralls: https://coveralls.io/builds/61287307

However, that doesn't change the following:

  • This worked before and now apparently doesn't work anymore. That makes it very likely that this is related to a change on the Coveralls side of things.
    I mean, there were, for this repo alone, > 15 successful PRs between the original change to the Coveralls native token and now and I've been using this same workflow pattern in multiple repos, some of which have had 100+ PRs since and they all were recognized correctly by Coveralls before.
  • If I remember correctly the GITHUB_TOKEN could not be used for private repos, so are those now out of luck ?
  • In earlier tickets, the advise often was to use the Coveralls token instead of the GH token.
  • In various places/other tickets, it has been said that using the COVERALLS_TOKEN will be required "in the near future".
  • The documentation states that the Coveralls token should be used (in various places, I'm listing a few below).

All in all, while changing back to the GITHUB_TOKEN solves my immediate problem, I do think the communication around the tokens may need a rethink ? (and the bug may need fixing).

Refs:

@afinetooth
Copy link
Collaborator

@jrfnl. You bet, my pleasure. Thanks for documenting the inconsistencies here. Personally, I am aware of that plan to switch to the Coveralls Repo Token, and that, at this time, we fall through, one to the other, if there's an issue with one, so, technically, both are supported. So I'm mystified as to why your previously working workflows stopped working with that 422 which, usually (but now, I suspect, not always) refers to that issue.

I have asked for a review by engineering here so we can discover more about what might have suddenly caused this not to work. I'll update you hear when that produces some further insights.

@jrfnl
Copy link
Author

jrfnl commented Jul 14, 2023

@afinetooth Thanks James.

I'm leaving that PR to change back to the GITHUB_TOKEN in draft for now as the PR through which I discovered this is not urgent to be merged, so let me know if I need to do a re-run of the workflow for either PR or if you'd want me to test something else.

@jrfnl
Copy link
Author

jrfnl commented Jul 14, 2023

Oh, just thinking.... all the PRs with passing builds between the change to the COVERALLS_TOKEN and now, were PRs by me (a repo admin), while this is a PR by the Dependabot. Could that have something to do with it ?

@jrfnl
Copy link
Author

jrfnl commented Jul 14, 2023

@afinetooth Found it!

Yes, looks like the PR coming from Dependabot is the reason for the failing build.

Dependabot is able to trigger GitHub Actions workflows on its pull requests and comments; however, certain events are treated differently.

For workflows initiated by Dependabot (...) using the pull_request...... events, the following restrictions apply:

  • GITHUB_TOKEN has read-only permissions by default.
  • Secrets are populated from Dependabot secrets. GitHub Actions secrets are not available.

Ref: https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#responding-to-events

While this explains the failing build, I do imagine this kind of thing might be a hindrance for adoption of the COVERALLS_TOKEN in the long run as this restriction means that workflows run for Dependabot PRs will always fail and if a repo has the Coveralls % as a required status, it makes those PRs unmergable.

I think this issue can now be closed, though hopefully it will help other people who may run into the same thing (including myself in the future...).

@afinetooth
Copy link
Collaborator

Hi @jrfnl. Helpful discovery!

So you're saying that you believe the only reason for the failing PR build was that it came from a dependabot PR (a fork)? And that access permissions to secrets was at play. That makes sense.

I was just reporting back with the results of the engineering review I requested (summarized):

  • We are not aware of any recent changed that could have caused this sudden change, outside, of, perhaps, a recent housecleaning effort that eliminated duplicate repos. In the off-chance that a duplicate of your repo existed, and that we deleted the wrong repo, it's possible that the remaining repo has the wrong repo token. (We think it's unlikely in your case because you repo had a significant build history, but still worth verifying your current repo token is correct.)
  • In general, though, repo resolution using the Coveralls Repo Token is now the simplest and the most straightforward method and should work as long as the correct token is provided.

So this explains why secrets.COVERALLS_TOKEN didn't work in the context of a dependabot PR, but secrets.GITHUB_TOKEN did.

As I mentioned above, we implemented the integration to try both, and to fall through to the other if one fails.

So, in any case, I was wrong to tell you to switch to GITHUB_TOKEN in general. You should be fine, and in fact have the preferred approach, by staying with COVERALLS_REPO_TOKEN, or in your case secrets.COVERALLS_TOKEN.

That does leave the question open of how to deal with forked PRs. Dependabot PRs seem like they have an inherent workaround (use secrets.GITHUB_TOKEN), but I'm not sure about the most secure way to handle general PRs from forks. Is that relevant to your needs right now?

jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Jul 15, 2023
Follow up on PR 468.

Turns out Dependabot PRs do not have access to secrets with the exception of (read-only) access to the `GITHUB_TOKEN`.

As the coverage test runs and the Coveralls status are required builds, this blocks Dependabot PRs from being merged without overruling the required statuses.

As I'd like to avoid that situation, I'm special casing Dependabot PRs for the token selection.

Refs:
* lemurheavy/coveralls-public#1721
* https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#responding-to-events
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Jul 15, 2023
Follow up on PR 468.

Turns out Dependabot PRs do not have access to secrets with the exception of (read-only) access to the `GITHUB_TOKEN`.

As the coverage test runs and the Coveralls status are required builds, this blocks Dependabot PRs from being merged without overruling the required statuses.

As I'd like to avoid that situation, I'm special casing Dependabot PRs for the token selection.

Refs:
* lemurheavy/coveralls-public#1721
* https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#responding-to-events
jrfnl added a commit to PHPCSStandards/PHPCSUtils that referenced this issue Jul 15, 2023
Follow up on PR 468.

Turns out Dependabot PRs do not have access to secrets with the exception of (read-only) access to the `GITHUB_TOKEN`.

As the coverage test runs and the Coveralls status are required builds, this blocks Dependabot PRs from being merged without overruling the required statuses.

As I'd like to avoid that situation, I'm special casing Dependabot PRs for the token selection.

Refs:
* lemurheavy/coveralls-public#1721
* https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#responding-to-events
@jrfnl
Copy link
Author

jrfnl commented Jul 15, 2023

So you're saying that you believe the only reason for the failing PR build was that it came from a dependabot PR (a fork)?

Correct.

That does leave the question open of how to deal with forked PRs.

Forked PRs in general should be fine. If I read things correctly, it's only the PRs from Dependabot which should be affected by this issue.

As I'd prefer to stay on the Coveralls token as the default, I'm going to change the workflows, as I prefer to have the least amount of "What is wrong with this PR, oh, hang on, I remember this issue" moments (and having to remember that kind of things for different repos and such).

Originally, I intended to change the token setting to:

${{ github.actor != 'dependabot[bot]' || secrets.COVERALLS_TOKEN && secrets.GITHUB_TOKEN }}

... but that doesn't seem to work as the expression will not evaluate to the token, but to true (unlike in other places in workflows where non-secret related expressions like this are used).

So instead, looks like I need to duplicate the steps to special case Dependabot. While not pretty, it's still better than having to remember there is an exception in play every time the repo receives a Dependabot PR.

I've updated PR PHPCSStandards/PHPCSUtils#490 to make that change instead of reverting the original PR to switch to the COVERALLS_TOKEN.

I've also confirmed this workflow logic works by letting Dependabot rebase the open PR after I merged the workflow change.


@afinetooth This might be something which needs to be called out in the documentation/example workflows.... (and probably good to mention it to the engineering team too as it may throw a spanner in the wheels of the switch to the COVERALLS_TOKEN).

One thing I'm left to wonder about its how to get this working for private repos, which IIRC can only use the COVERALLS_TOKEN, not the GITHUB_TOKEN.... ?

@jrfnl
Copy link
Author

jrfnl commented Jul 19, 2023

That does leave the question open of how to deal with forked PRs.

Forked PRs in general should be fine. If I read things correctly, it's only the PRs from Dependabot which should be affected by this issue.

I stand corrected. Just ran into the a 422 error for a PR from an external contributor... this is not good...

@afinetooth
Copy link
Collaborator

@jrfnl , darn it. I'm sorry to hear that.

Our engineering team won't be available to help for a few days, but if you want to share the URL for the action run (and, ideally, a verbose build log), I'll be happy to have a look. (I could not find it in your actions history, just the failed dependabot PRs.)

Question:
Have you tried using the Coveralls Github Action on its own, without combining it with coveralls-python? I ask because, since our Action's underlying integration (the Coverage Reporter) supports pytest cov format coverage reports, you should be fine. I would love to know if that's the case because you would be removing any cross-compatibility issues and significantly reducing the complexity of your setup by dropping down to the one integration. We could also better support you, I feel, just because we maintain the Action and the Coverage Reporter.

coveralls-python is a great integration and well-maintained, it's just that, clearly, we haven't done the best job of coordinating with the maintainer to make sure that Coveralls, and our official integrations, are always backwards-compatible. I know for a fact that we consider that integration when we make changes, and try to avoid potential issues, but, like I say, we have definitely not nailed it.

@jrfnl
Copy link
Author

jrfnl commented Jul 20, 2023

Our engineering team won't be available to help for a few days, but if you want to share the URL for the action run (and, ideally, a verbose build log), I'll be happy to have a look. (I could not find it in your actions history, just the failed dependabot PRs.)

Yes, sorry, different repo, same setup.

This was the PR: PHPCSStandards/PHPCSExtra#258
And here is the failed actions run: https://github.com/PHPCSStandards/PHPCSExtra/actions/runs/5604158260/job/15183403404

Question: Have you tried using the Coveralls Github Action on its own, without combining it with coveralls-python? I ask because, since our Action's underlying integration (the Coverage Reporter) supports pytest cov format coverage reports, you should be fine. I would love to know if that's the case because you would be removing any cross-compatibility issues and significantly reducing the complexity of your setup by dropping down to the one integration. We could also better support you, I feel, just because we maintain the Action and the Coverage Reporter.

I'm not using coveralls-python, but php-coveralls which is (AFAIK) not (yet) supported by the Coverage Reporter.

The problem is that the standard output from PHPUnit cannot be handled by Coveralls, so it needs to be converted from the clover format to a format Coveralls can deal with.

Would be nice to be able to drop those steps, but I don't think it's an option at this time.

@afinetooth
Copy link
Collaborator

afinetooth commented Jul 21, 2023

@jrfnl Oh, shoot. I'm so sorry. I confused this momentarily with another issue from a coveralls-python user with practically the same behavior. My bad.

Actually, PHP/Clover XML format is the next on our list of formats to support with the Coverage Reporter. We've just been challenged finding the time to get to it.

There might be an option right now, if you're using PHPUnit, which is to export to Cobertura XML format, which the Coverage Reporter currently supports.

I have also raised the priority of our own internal task to add PHP/Clover XML support, so if I get an ETA I'll update you here.

In the meantime, I realize you are an OpenSource user, but if you are inclined to contribute a new parser (official support for a coverage report format) to the Coverage Reporter, we offer a free month of paid service, which we'd also be happy to donate to someone of your choice. Here's an example of a parser, for Cobertura XML format. You can see the other parsers here.

@jrfnl
Copy link
Author

jrfnl commented Jul 21, 2023

@jrfnl Oh, shoot. I'm so sorry. I confused this momentarily with another issue from a coveralls-python user with practically the same behavior. My bad.

@afinetooth No worries, I actually referred to a similar issue with coveralls-python in the original report as I realized how similar the issues were.

Actually, PHP/Clover XML format is the next on our list of formats to support with the Coverage Reporter. We've just been challenged finding the time to get to it.

That would be amazing!

There might be an option right now, if you're using PHPUnit, which is to export to Cobertura XML format, which the Coverage Reporter currently supports.

Unfortunately, using the Cobertura format doesn't look to be a realistic option.

From what I can see, that reporting option is available since PHPUnit 9.4, while these projects are open source and support a wide range of PHP versions, which, in this case, means the tests need to run on PHPUnit 4.x - 10.x and code coverage will generally be created using high/low builds to get the coverage report correct, so just sending in the "high" builds, which have access to the Cobertura report format, is not really an option.

I also imagine that mixing the two formats (high using Cobertura, low using Clover) may not be optimal and would make the workflows even more complex than they already are.

It might be an option soonish for one of my customers (which has a Pro account), but at this time, they still support PHP 7.2, so for high/low builds they still need PHPUnit 8.x. I'll explore this option for them once they've dropped support for PHP 7.2 (which is planned some time over the next year).

I have also raised the priority of our own internal task to add PHP/Clover XML support, so if I get an ETA I'll update you here.

👍🏻

In the meantime, I realize you are an OpenSource user, but if you are inclined to contribute a new parser (official support for a coverage report format) to the Coverage Reporter, we offer a free month of paid service, which we'd also be happy to donate to someone of your choice. Here's an example of a parser, for Cobertura XML format. You can see the other parsers here.

If wish I could find the time for it, but I'm overloaded (and underpaid) as is ;-)

@afinetooth
Copy link
Collaborator

HI @jrfnl:

Unfortunately, using the Cobertura format doesn't look to be a realistic option.

Got it! Makes sense. No problem. We'll see when we can complete the PHP/Clover XML support.

If wish I could find the time for it, but I'm overloaded (and underpaid) as is ;-)

I can relate! No worries, maybe some rich PHP developer with a lot of time on their hands will come along in the next few weeks. LOL. 🎠

@jrfnl
Copy link
Author

jrfnl commented Jul 21, 2023

maybe some rich PHP developer with a lot of time on their hands will come along in the next few weeks. LOL. 🎠

By the looks of the specs though, you don't need a PHP dev, but a Crystal one. (which is one of the blockers for me to even contemplate working on this, I'd never even heard of Crystal until now, so while I could probably get the basics done with some copy/paste trial and error, I don't have a test environment, will probably create parse/compile errors, definitely won't write things in a "smart" way etc etc Learning a new language from scratch is not something I currently have time for.)

@afinetooth
Copy link
Collaborator

By the looks of the specs though, you don't need a PHP dev, but a Crystal one.

Very true! My bad. I was thinking more about the format (PHP/Clover XML) than the source code for the Reporter itself, which, as you say, is written in Crystal, which is like Ruby with a type system, which makes it easier/safer to compile to C.

Appreciate you even considering it!

It's on the near horizon. I have added it as a hopeful to our next sprint backlog, but it's probably more likely to be part of the next one. I'll be sure you update you here as soon as I have an ETA.

@jrfnl
Copy link
Author

jrfnl commented Jul 27, 2023

It's on the near horizon. I have added it as a hopeful to our next sprint backlog, but it's probably more likely to be part of the next one. I'll be sure you update you here as soon as I have an ETA.

That would be awesome! I look forward to being able to switch over soon(ish) (presuming your sprints are not three months long 😂 )

@afinetooth
Copy link
Collaborator

LOL. Our sprints are 2-weeks each. Once it's in the queue it shouldn't be too long.

@jrfnl
Copy link
Author

jrfnl commented Aug 25, 2023

@afinetooth Just ran into the 422 error again with an external contributor, so thought I'd check in how things are progressing ?

@afinetooth
Copy link
Collaborator

Hi @jrfnl. While we didn't get to it in our last sprint, it's in this one and currently assigned to a team member, so I've checked in so get a sense of when we'll start it and ETA. Will update you when I know something.

@afinetooth
Copy link
Collaborator

afinetooth commented Aug 30, 2023

Hi @jrfnl. Still don't have an exact date but I'd guess ETA is this week as we have a WIP PR that looks pretty close and should go to review this week. We would love it if you'd help us test in production. If you can do that, I'll connect you with our engineer who worked on it to respond to your feedback after release. Will update you.

@jrfnl
Copy link
Author

jrfnl commented Sep 15, 2023

@afinetooth @iurev Please let me know if there is anything else you'd like me to test. If not, I'll proceed with switching out all repos within my purview to the new workflow setup for uploading to Coveralls over the next few days.

@afinetooth
Copy link
Collaborator

afinetooth commented Sep 15, 2023

@jrfnl Thanks for the updates!

Before you start the switchover, it would be ideal to let us review your results and also post the updates that will go into our documentation. We have a draft right now, but there were a couple of decisions made that were different than expected.

The updates will make those clear, and from that point your switchover steps will be obvious and, we hope, simple.

Will do my best to get you that today so you can proceed as soon as Mon.

@jrfnl
Copy link
Author

jrfnl commented Sep 15, 2023

@afinetooth Of course, take your time and please let me know if you'd like me to review any of the documentation updates.

@jrfnl
Copy link
Author

jrfnl commented Sep 20, 2023

@afinetooth Just checking in to see how things are progressing on your end ?

@afinetooth
Copy link
Collaborator

HI @jrfnl. We're a little pressed for time here due to incidents this week, working through a backlog of support requests. We still have that draft though and had more conversation on it, so I hope to work on it and share something tomorrow. Will at-mention you.

@jrfnl
Copy link
Author

jrfnl commented Sep 21, 2023

@afinetooth Understood. Good luck with getting the incidents resolved.

@jrfnl
Copy link
Author

jrfnl commented Oct 4, 2023

@afinetooth Just checking in to see how things are going... ?

@jrfnl
Copy link
Author

jrfnl commented Oct 24, 2023

@afinetooth Anything I can do to move this forward ? How can I help ?

@afinetooth
Copy link
Collaborator

@jrfnl Thanks, I appreciate you checking back and apologize for the lack of response.

If you don't mind me asking for a little help re-orienting around the exact deliverables to help you proceed, I plan to carve out a couple of hours this Fri to complete them for you.

To review:

On Sep 14 you had said:

@afinetooth @iurev Please let me know if there is anything else you'd like me to test. If not, I'll proceed with switching out all repos within my purview to the new workflow setup for uploading to Coveralls over the next few days.

To which, on Sep 21, I had replied:

Before you start the switchover, it would be ideal to let us review your results and also post the updates that will go into our documentation. We have a draft right now, but there were a couple of decisions made that were different than expected.

The updates will make those clear, and from that point your switchover steps will be obvious and, we hope, simple.

So my understanding of what I hoped to provide as a deliverable is a final list of updated recommendations, which also take into consideration both Dependabot PRs and forked PRs.

I have that, in the form of my last draft, which I wanted to review again before sharing it because it pivots on the earlier recommendation, from us, to use the COVERALLS_REPO_TOKEN when and where possible.

In order to further justify that reversal, I was hoping to compare my recommendations to each of your test cases above to verify they work with the new recommendations, and why, but, TBH, that is the effort I am struggling to find time for.

So here is a proposal:

What if I share my latest draft of updated recommendations, along with excerpts of our internal engineering conversation, to the extent they are helpful / illuminating?

Then, at least, you can get a sense as to why our basic recommendation is to continue using the GITHUB_TOKEN, and you can then verify whether our follow-on recommendations work with the rest of your test cases (your previous tests).

So far, I have only addressed the first of those cases:

Public repo (with Forked PR)

  • As reported previously: when a PR is based on a branch from the same repo and the COVERALL_TOKEN is used, all works fine.
  • However, this does not work for PRs from forks.
  • Removing the github-token input from the steps using the coverallsapp/github-action solves that.
    This has been tested now via GH Actions/coverage: remove COVERALLS_TOKEN PHPCSStandards/PHPCSUtils#515 (removed the github-token input) in combination with an open PR from a fork.
  • Status unknown/still to be tested: Dependabot PRs.

At least for this case, I know our recommendations to work for that case because removing the github-token: whatever entry from your config causes the Coveralls GitHub Action resort to it's default, which is the GITHUB_TOKEN.

I'll strike a line here to separate the updated recommendations:


Recommendations

Summary

We are basically back where we have always been in terms of recommending that users of the Coveralls GitHub Action always use the GITHUB_TOKEN to authenticate their repos with the Coveralls API.

Furthermore, as this is the default behavior of the Coveralls GitHub Action, users need do nothing to select this token by which their repo will be authenticated by the Coveralls API.

The only caveat here is that, for this to work, the incoming service_name parameter must be github and not changed to anything else using the COVERALLS_SERVICE_NAME env var.

Follow-On Recommendations

For Dependabot PRs:

Workflow runs triggered by Dependabot PRs run as if they were from a forked repository, so follow the recommendations for forked repositories.

For Forked PRs:

Other important details:

  • These recommendations apply when your integration will set the service_name parameter to github only. The Coveralls GitHub Action will always set the service_name parameter to github, but if you are using GitHub Actions with any other integration besides the Coveralls GitHub Action, you may need to verify through a test, or manually configure your integration, to make sure it is sending service_name: github. The way to do that is by setting the this Coveralls Environment Variable as follows: COVERALLS_SERVICE_NAME: github.
  • If your integration sets service_name to any other value besides github, or if you have manually configured COVERALLS_SERVICE_NAME to some other value, such as github-actions, then you will need to authenticate your coverage reports using the COVERALLS_REPO_TOKEN, and, to keep that token secure, your Forked PRs will need access to secrets through one of these permission level settings, or through Dependabot Secrets for Dependabot PRs.
  • If you are using GitHub Actions for CI but are using any other Coveralls Integration besides the Coveralls GitHub Action, then we recommend setting your Coveralls Repo Token to the action's GitHub Token, like this in your Coveralls step(s): COVERALLS_REPO_TOKEN: ${{ github.token }}.

Conversation

The internal engineering conversation that resulted in the above recommendations:

Please note: Names and handles have been redacted to protect the innocent.

Slack Thread on New Recommendation for WHICH TOKEN to use with Coveralls GitHub Action

@jrfnl
Copy link
Author

jrfnl commented Oct 26, 2023

@afinetooth Thanks for getting back to me.

So my understanding of what I hoped to provide as a deliverable is a final list of updated recommendations, which also take into consideration both Dependabot PRs and forked PRs.

Well, that + updated documentation on the Coveralls website based on those recommendations ?

To Public Repos - Workflow runs triggered by Forked PRs (and Dependabot PRs) use a read-only GITHUB_TOKEN. The GITHUB_TOKEN with read-only access should be sufficient for authenticating your coverage report with the Coveralls API. <-- (Note: I'm not actually 100% sure that's true. Sounds correct but needs to be tested.)

AFAICS this is correct as demonstrated via this PR: PHPCSStandards/PHPCSUtils#514

Other than that, the recommendations so far look good. I think the most important bit for the updated documentation is to recommend to always use the GITHUB_TOKEN, including for private repos, which is in stark contrast to what was previously needed (the COVERALLS_TOKEN) and recommended.

Just to reiterate what I have tested with the GITHUB_TOKEN + Coveralls native GitHub Action runner:

Repo type Repo local PR Dependabot PR PR from fork
public ✔️ ✔️ ✔️
private ✔️ ✔️

In contrast to that, using the COVERALLS_TOKEN resulted in:

Repo type Repo local PR Dependabot PR PR from fork Remark
public ✔️
private ✔️ Repo local PRs were previously ❌ when using the GITHUB_TOKEN.

Also note that Dependabot PRs are a special breed. There are repo local PRs, not PRs from forks, but get special treatment in GitHub Actions.

Hope this helps. Let me know if you want me to read through/review further texts.

@jrfnl
Copy link
Author

jrfnl commented Nov 1, 2023

@afinetooth Any update ?

@jrfnl
Copy link
Author

jrfnl commented Nov 1, 2023

Also wondering about the value to use for format after seeing https://github.com/coverallsapp/coverage-reporter/pull/102/files

As per #1721 (comment), I've used format: clover in my test runs and that worked, though might have worked only due to the auto-discovery ?

@jrfnl
Copy link
Author

jrfnl commented Nov 12, 2023

@afinetooth Ping ?

jrfnl added a commit to PHPCSStandards/PHPCSExtra that referenced this issue Nov 12, 2023
Simplify the code coverage workflow by removing the dependency on the `php-coveralls/php-coveralls` package and switching to the `coverallsapp/github-action` action runner, which, as of the release of the [0.6.5 version of the Coverage Reporter](https://github.com/coverallsapp/coverage-reporter/releases/tag/v0.6.5) now natively supports the Clover format.

The `COVERALLS_TOKEN` can now be removed from Settings -> Secrets.

Ref: lemurheavy/coveralls-public#1721
@jrfnl
Copy link
Author

jrfnl commented Nov 17, 2023

@afinetooth I'm getting to the point where I'm done waiting. It's been two months since I ran those test scenarios.

You asked me to hold off from using this workflow for the time being while you double-checked things, but in the mean time repos are still running into the issues described above, so I would really really really like to start using the new setup...

@afinetooth
Copy link
Collaborator

@jrfnl Im truly sorry for the miscommunication. I was under the impression you were using the new workflow. Have you tried that? Or are you still waiting on confirmation? We are go on the new workflow, we are just behind on updating documentation.

My goal at this point would be to get you on the new workflow and make sure you're not having any issues with it.

Can you please try and confirm either way.

@jrfnl
Copy link
Author

jrfnl commented Nov 24, 2023

@jrfnl Im truly sorry for the miscommunication. I was under the impression you were using the new workflow. Have you tried that?

@afinetooth I am for the repo which was used to run the tests...

Or are you still waiting on confirmation?

... but was waiting for confirmation before updating the workflows in all the other repos as you requested.

We are go on the new workflow, we are just behind on updating documentation.
My goal at this point would be to get you on the new workflow and make sure you're not having any issues with it.

I have the branches ready to go locally, so I'll pull & merge over the weekend.
Will ask people to report issues if they see any.

@jrfnl
Copy link
Author

jrfnl commented Nov 24, 2023

@afinetooth I've started pulling the changes and the workflows are fine, but I am seeing some changes in the reported coverage. In most cases, the changes are small, but sometimes they are more significant.

Looks to be related to a difference in line counts between php-coveralls and the Coveralls native reporter.

This might be an interesting build to have a look at: https://coveralls.io/builds/64166567 (coverage changed gives the best view):

image

@afinetooth
Copy link
Collaborator

@jrfnl Yes, this is a known issue we're investigating right now, likely a regression from a recent release.

Hoping for a quick fix by Mon.

Thanks for the example build. 🙏

@jrfnl
Copy link
Author

jrfnl commented Nov 25, 2023

@jrfnl Yes, this is a known issue we're investigating right now, likely a regression from a recent release.

@afinetooth In case it helps: I have the feeling (though haven't dug in deep to confirm) that it is the function name(...) { lines being counted which is throwing things off.

@afinetooth
Copy link
Collaborator

@jrfnl Ok, thanks for that. I'll look at your coverage report to confirm.

Do you not see function definitions being counted as relevant in your native reports? (I believe there's a debate about that.)

I'll share a JSON version so you can compare.

@jrfnl
Copy link
Author

jrfnl commented Nov 25, 2023

Do you not see function definitions being counted as relevant in your native reports? (I believe there's a debate about that.)

@afinetooth This is a screenshot from the PHPUnit native HTML report (for another package) and as you can see, the function declaration lines itself are not counted:

image

@afinetooth
Copy link
Collaborator

@jrfnl Haven't had a chance to look yet, but sharing the reports for this build in case it's helpful:
https://coveralls.io/builds/64166567

6985357654-coverage-reports.zip

@afinetooth
Copy link
Collaborator

@jrfnl Just an update: I created an Engineering ticket for this and, while we're still working through a backlog from the holiday, someone should at least look at it today, so I'll get you an update as soon as I see one.

@afinetooth
Copy link
Collaborator

@jrfnl I just got to examining this and, unfortunately, it looks like the coverage reports you exported were our already-converted ones (they match the ones processed by our API as you'll see below):

Screenshot 2023-12-11 at 2 25 22 PM

I understand the original clover reports may not still be available, but if you do have some I'd be happy and interested to compare them.

I did some digging around for example clover format reports, just to see how they were parsed by our clover parser, and I was not surprised to see that, in our tests, some of our test data came from one of your (public) projects.

Here's that test fixture file. I'm not sure if it's full and identical to what must have been taken from a locally-run coverage report, but I did see that it was created around Sep 1, so I found the nearest Source File Report at Coveralls:
https://coveralls.io/builds/62230525/source?filename=PHPCSUtils%2FAbstractSniffs%2FAbstractArrayDeclarationSniff.php#L178

And I did notice that, back then, the report was not marking function declarations as relevant:

Screenshot 2023-12-11 at 2 51 30 PM

But recent reports, as we know, do:
https://coveralls.io/builds/64474766/source?filename=PHPCSUtils%2FAbstractSniffs%2FAbstractArrayDeclarationSniff.php#L178

Screenshot 2023-12-11 at 2 52 46 PM

So, depending on the date you switched to the Coveralls GitHub Action, we can probably prove that our clover parser is misprocessing those type="method" lines:

<?xml version="1.0" encoding="UTF-8"?>
<coverage generated="1693391816">
  <project timestamp="1693391816">
    <file name="/home/yu/projects/PHPCSUtils/PHPCSUtils/AbstractSniffs/AbstractArrayDeclarationSniff.php">
      <class name="PHPCSUtils\AbstractSniffs\AbstractArrayDeclarationSniff" namespace="global">
        <metrics complexity="44" methods="3" coveredmethods="3" conditionals="0" coveredconditionals="0" statements="79" coveredstatements="79" elements="82" coveredelements="82"/>
      </class>
      <line num="178" type="method" name="process" visibility="public" complexity="4" crap="4" count="13"/>
      <line num="181" type="stmt" count="13"/>
      <line num="182" type="stmt" count="1"/>
      <line num="184" type="stmt" count="1"/>

      [...]

      <line num="537" type="stmt" count="6"/>
      <line num="538" type="stmt" count="3"/>
      <line num="541" type="stmt" count="3"/>
      <metrics loc="552" ncloc="237" classes="1" methods="3" coveredmethods="3" conditionals="0" coveredconditionals="0" statements="79" coveredstatements="79" elements="82" coveredelements="82"/>
    </file>
    <file name="/home/yu/projects/PHPCSUtils/PHPCSUtils/BackCompat/BCFile.php">
      <class name="PHPCSUtils\BackCompat\BCFile" namespace="global">
        <metrics complexity="96" methods="13" coveredmethods="13" conditionals="0" coveredconditionals="0" statements="244" coveredstatements="244" elements="257" coveredelements="257"/>
      </class>
      <line num="99" type="method" name="getDeclarationName" visibility="public" complexity="15" crap="15" count="34"/>
      <metrics loc="858" ncloc="400" classes="1" methods="13" coveredmethods="13" conditionals="0" coveredconditionals="0" statements="244" coveredstatements="244" elements="257" coveredelements="257"/>
    </file>
    <metrics files="37" loc="11201" ncloc="5558" classes="36" methods="182" coveredmethods="163" conditionals="0" coveredconditionals="0" statements="2420" coveredstatements="2376" elements="2602" coveredelements="2539"/>
  </project>
</coverage>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants