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

Missing comments in PRs #1723

Closed
Wirone opened this issue Jul 19, 2023 · 8 comments
Closed

Missing comments in PRs #1723

Wirone opened this issue Jul 19, 2023 · 8 comments

Comments

@Wirone
Copy link

Wirone commented Jul 19, 2023

Recently I've found out that Coveralls stopped adding comments to PHP CS Fixer's pull requests. After further investigation it turns out that it happened at the beginning of April. When looking on [this list of PRs] you can see that first one has the comment, second one does not have it, but comment can be found on some PRs created later (like this one).

I've read the FAQ and verified that we do build for base branch. Every PR and default branch has Github Action job with coverage, which is sent correctly to your service (default branch, some latest PR). Unfortunately there is no summary comment in the PRs.

I believe we did not do any change related to repository permissions and/or CI process related to Coveralls. Can you provide information why comments are not working anymore?

@afinetooth
Copy link
Collaborator

afinetooth commented Jul 21, 2023

Hi @Wirone.

tl;dr

Coveralls is hitting a rate limit error for your repo, which probably occurs just by the time we try to send a PR comment for a given PR build. (I see it in our logs and it's happened a lot.)

Your repo needs an owner, which should probably be a maintainer. If you could send me an appropriate GitHub username, that would be great; but, for now, I've made myself the owner, so please let me know if that doesn't fix it.

Please note the Update at bottom, as there's a way to get around this repo ownership issue altogether.

Join me for a voyage to discovery, below, if you're so inclined.

Details

If your repo is set up to receive PR comments, per the instructions here---which I assume it is, or else you wouldn't have received any thus far---and we have a base build for each PR that should have received a comment, then I'm not quite sure what the issue could be.

I suspect something related to permissions. Before we start looking in that direction, though, I wanted to test sending some PR comments now, for some PR builds that didn't receive them before.

Here's one that didn't previously have a PR comment:
https://coveralls.io/builds/60936076

And here's the PR with the comment, created just a couple of minutes ago:
PHP-CS-Fixer/PHP-CS-Fixer#7080

Screenshot 2023-07-20 at 4 25 54 PM

So, there doesn't seem to be anything preventing us from sending those. It suggests something real-time in the environment preventing the send.

Same thing for the example you shared, that didn't receive a PR comment:
PHP-CS-Fixer/PHP-CS-Fixer#6883

I was able to send one manually, so now it has a comment, at the bottom, from about a minute ago.

What I am seeing is a bunch of rate limit errors from Github, in our production bug tracking system, for your repo. Which is really strange (the IP address is ours):

GET https://api.github.com/repos/PHP-CS-Fixer/PHP-CS-Fixer/commits/82b04ac2abafea0ebe87bade69dadeaa85967089/statuses: 403 - API rate limit exceeded for 54.167.168.237. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.) See: https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting

Your requests shouldn't be unauthenticated, so it makes me thinks your repo owner's token is expired, or problematic, and needs to be refreshed.

Aha. Your repo doesn't have an owner. That explains it. We can get through to a public repo as any user, but there's a limit on those requests if we're not passing a token.

I need to give the repo an owner (our legacy GitHub integration, which that org is on, is based on an OAuth app, which needs to leverage a user token), so I was going to make you the owner, but I don't see a Coveralls user account for your GitHub username. If you want to go to https://coveralls.io and sign and sign in via GitHub, then let me know here (or at support@coveralls.io), I will make you the owner. Then requests will leverage your Coveralls OAuth token.

Or, if you know the maintainer, you can also share their GitHub username and I can see if they have a Coveralls account, as it would be reasonable to make them the owner. (I could not find an account for @PHP-CS-Fixer.)

Otherwise, I think we'll continue hitting the API rate limit error for unauthenticated requests.

For the time being, I'll make myself the owner and we'll see if it resolves the situation.

Please let me know if it does!


Update

Another option is that an owner of the org (PHP-CS-Fixer) can migrate to our new GitHub integration, which is in beta, which relies on a GitHub App, which uses its own token and doesn't need to leverage a user token, which they can sign up for here.

@Wirone
Copy link
Author

Wirone commented Jul 21, 2023

@afinetooth thank you very much for your time and explanations! The lead maintainer of PHP-CS-Fixer is @keradus (currently on vacation, so don't be scared by the status 😅) so if you could set him as a repo owner on your side, it would be great 🙂. We talked about migrating to Github App integration and we'll try to do it, but for now it would be good to have Coveralls running as is, hence this issue. Once again, thank you for your assist.

@afinetooth
Copy link
Collaborator

afinetooth commented Jul 27, 2023

Hi @Wirone. I attempted to make @keradus the owner of the repo, but currently, his token is expired, I assume from being away on vacations. Typically a session will last 30-days without signing out.

I'll keep myself as the owner. Just ping me (or ask @keradus to, when he's back), and I'll replace myself with him. I'll keep this open for now.

@keradus
Copy link

keradus commented Aug 18, 2023

Hey, I'm back, thanks for your support on it, @afinetooth!

I need to give the repo an owner (our legacy GitHub integration, which that org is on, is based on an OAuth app, which needs to leverage a user token), so I was going to make you the owner, but I don't see a Coveralls user account for your GitHub username. If you want to go to https://coveralls.io and sign and sign in via GitHub, then let me know here [...], I will make you the owner. Then requests will leverage your Coveralls OAuth token.

I logged in on coveralls. Anything more I should do on my side?

our legacy GitHub integration, which that org is on (...)
Another option is (...) migrate to our new GitHub integration, which is in beta,

is there anything not-yet-legacy, but already-not-beta to use? 😅

@afinetooth
Copy link
Collaborator

Hi @keradus! My pleasure.

I logged in on coveralls. Anything more I should do on my side?

Nope. I made you the repo owner. If there's ever an issue receiving PR comments or status updates (with the legacy integration), all you'll probably need to do is login to Coveralls.io to fix it.

is there anything not-yet-legacy, but already-not-beta to use? 😅

So, technically no, but the beta has been up-and-running since May 1 and is very stable.

Once you're signed up and the feature is on, migration only takes a couple of minutes. We would love to have some more OSS users on it. :)

Beta schedule:

Open beta in Sep:
We are behind on communication about it, but we intend to make the beta "open" in Sep. Which, in our case, just means that all users will see an in-app notification with the BEGIN MIGRATION dialog at the top of their ADD REPOS page.

Mandatory switchover by end of Oct:
We are targeting a mandatory switchover by end of Oct, which, if you're interested, just means that, on that date, if you haven't already migrated, you'll have to login through the GitHub App and choose which repos to make available to Coveralls (and that flow will have all your currently active repos pre-selected for you to make it a one-click process).

Last details:

  • Migration leads should be org owners
  • You can see more about the process here

@Wirone
Copy link
Author

Wirone commented Aug 20, 2023

I believe we can close it now as it works as expected. Thank you very much @afinetooth for your help 🍻!

@Wirone Wirone closed this as completed Aug 20, 2023
@keradus
Copy link

keradus commented Aug 21, 2023

big thanks !

@afinetooth
Copy link
Collaborator

Great, thanks for the update, guys.

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