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

Update to check for PR status #228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dev-akshaygupta
Copy link

Resolves #225

@bmispelon
Copy link
Member

Hi and thanks for sending us this PR! ✨

Before I review it more thoroughly, can you explain how you found which status were possible? You used error, building and success, but are those the values we actually get from the API? Also, are there other values that could be returned from the API, or only these 3?
A link to the relevant documentation would be helpful here for reviewing.

Thanks!

@dev-akshaygupta
Copy link
Author

dev-akshaygupta commented Nov 8, 2024

I assumed that what you have is mentioned in the description of the bug is what needs to checked for status. My bad!

After further looking into Github Docs, I found these four to be the right pick for status check - error, failure, pending, success.

https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28

If this looks ok to you. I can add these for status check.

@bmispelon
Copy link
Member

Hi 👋🏻

Sorry for the lack of activity on this, I haven't forgotten about it 😁
I started digging a bit into the documentation (big thanks for the link by the way, that saved me time) and discovered that GitHub has three separate APIs and they all give different results:

  • commit statuses (what we use today, even though we only use the first result)
  • check runs (what the PR page uses I think)
  • test suites (not sure what that is)

In my pull request, the PR page showed an error, but all the commit statuses returned by the API call were success. I think we should switch to using the check runs API, but the return values from that one is more complex because you have two fields: status and conclusion and both of them need to be taken into account.

Anyway, as a result of this I've been a bit stuck trying to decide what the best path forward is. Do you have experience with using those APIs? Do you think one makes more sense than another?

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

Successfully merging this pull request may close these issues.

PR status only take the first check into account
2 participants