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

Query CI providers for PR number #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

Problem: in the GH status hooks there's no reference of the PR number and the API also doesn't provide a way to match the commit sha to a respective PR. However most CI providers maintain a reference on their site, so we can simply query them. This main part of this PR implements this.

Moreover it adds the following check to the GH Hook handler as a proof-of-concept to show that fetching the PR number works. Future work can extend these checks arbitrarily:

IF status == "success"
-> query GH API for all current CI statuses
IF two or more are "success"
-> figure our PR number (abort on not found)
-> query GH review API
IF no reviews found
-> add "needs review" label
IF review found
-> remove "needs review"

Other

  • added addAPIExpections
  • fixed bug in payloadServer (somehow writing a Json response with toString triggered undefined behavior)
  • made the PRReview check optional in the test suite (it doesn't make sense to update all tests with their exact API expectations all the time)

@wilzbach
Copy link
Member Author

Another benefit of this is that we can detect failures by a CI and connect them to a PR, which should be quite interesting as the auto-tester tries to rebuild PRs constantly and thus will let us know if the PR isn't buildable, mergeable etc.

@WebDrake
Copy link

One question here: is it all possible to be more precise about what CI are allowed to pass?

I would suggest that some CI are more important than others -- so e.g. style CI should not block moving to 'needs review', but test-suite failure should probably see the tag set to 'needs work' (maybe with a friendly message about how to address the test-suite failure the first time it happens).

@wilzbach
Copy link
Member Author

I would suggest that some CI are more important than others

Hehe at the moment GH is still in the "all CIs are equal stage", maybe soon we will have "all CIs are equal, but some are more equal" ;-)

One question here: is it all possible to be more precise about what CI are allowed to pass?

AFAIK this is not exposed via the GH API and I can think of these three options:

So yeah I absolutely agree with you about this, but I wanted to keep this already large PR as small as this is really just intended to be able to know for what PR we just received an event which is the first requirement to do anything like this.

(I have opened #58 for this)

case "CyberShadow/DAutoTest":
prNumber = checkDTest(url);
break;
case "Project Tester":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the new Jenkins, should be continuous-integration/jenkins/pr-merge.

@MartinNowak
Copy link
Member

Quite unfriendly of GH to not do the translation, guess it's b/c a single commit status could apply to multiple PRs.
In the longer run keeping track of sha1 -> PR relations ourselves would allow to avoid maintaining that integration.

@CyberShadow
Copy link
Member

I think at this point it would be simpler to store locally which commit SHA1s belong to which PRs (when a PR is created or synchronized).

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.

None yet

4 participants