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

Add 'head_branch' to GitHubChecksRerunActionCause #223 #226

Merged
merged 7 commits into from
Dec 21, 2021

Conversation

jgedarovich
Copy link
Contributor

@jgedarovich jgedarovich commented Dec 13, 2021

Pull the 'head_branch' name out from the check_runs check_suite head_branch property, add it to the build cause along side an accessor for it.

Issue: #223

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jgedarovich jgedarovich requested a review from a team as a code owner December 13, 2021 22:04
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #226 (2b51e56) into master (8540dd7) will increase coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head 2b51e56 differs from pull request most recent head aa812ff. Consider uploading reports for the commit aa812ff to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #226      +/-   ##
============================================
+ Coverage     79.65%   79.88%   +0.23%     
  Complexity      168      168              
============================================
  Files            16       16              
  Lines           516      522       +6     
  Branches         47       47              
============================================
+ Hits            411      417       +6     
  Misses           83       83              
  Partials         22       22              
Impacted Files Coverage Δ
...ugins/checks/github/CheckRunGHEventSubscriber.java 95.74% <100.00%> (+0.62%) ⬆️
...ins/plugins/checks/github/GitHubChecksContext.java 100.00% <0.00%> (ø)
...ins/plugins/checks/github/GitHubChecksDetails.java 76.34% <0.00%> (ø)
...ins/plugins/checks/github/GitSCMChecksContext.java 58.33% <0.00%> (ø)
...s/plugins/checks/github/GitHubChecksPublisher.java 100.00% <0.00%> (ø)
...cks/github/status/GitSCMStatusChecksExtension.java 76.92% <0.00%> (ø)
...ks/github/status/GitHubStatusChecksProperties.java 95.45% <0.00%> (ø)
...ithub/status/GitHubSCMSourceStatusChecksTrait.java 75.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8540dd7...aa812ff. Read the comment docs.

@timja
Copy link
Member

timja commented Dec 14, 2021

I think ok but can you sort checkstyle issues, yes the config is weird here…

@timja timja requested review from XiongKezhi and uhafner December 14, 2021 19:41
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Looks good to me. The PMD warning will automatically be fixed if you combine the try blocks.

LOGGER.log(Level.INFO, "Received rerun request through GitHub checks API.");
try (ACLContext ignored = ACL.as(ACL.SYSTEM)) {
scheduleRerun(checkRun);
scheduleRerun(checkRun, branchName);
Copy link
Member

Choose a reason for hiding this comment

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

The actual code that does something useful would be simpler to find if you would use one try-catch block for the whole method (calls in line 73 and 86), then you can integrate the variable assignments in the declaration.

Copy link
Member

Choose a reason for hiding this comment

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

I changed the first part of the code in #227.

@uhafner uhafner added the enhancement New feature or request label Dec 16, 2021
@timja
Copy link
Member

timja commented Dec 16, 2021

conflicted now

@jgedarovich
Copy link
Contributor Author

thanks for quick response - i should have some time tomorrow to update this to both fix the conflict and to take the excellent recommendation of uhafner

Jenkinsfile Outdated Show resolved Hide resolved
@timja timja merged commit c474ce9 into jenkinsci:master Dec 21, 2021
try {
GHEventPayload.CheckRun checkRun = GitHub.offline().parseEventPayload(new StringReader(payload), GHEventPayload.CheckRun.class);

JSONObject payloadJSON = new JSONObject(payload);
branchName = payloadJSON.getJSONObject("check_run").getJSONObject("check_suite").getString("head_branch");
Copy link
Member

@timja timja Jan 15, 2022

Choose a reason for hiding this comment

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

looks like the head_branch can be null:
jenkins-infra/helpdesk#2738 (comment)

https://docs.github.com/en/rest/reference/checks#suites

Note: The Checks API only looks for pushes in the repository where the check suite or check run were created. Pushes to a branch in a forked repository are not detected and return an empty pull_requests array and a null value for head_branch.

it's null for forks

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

Successfully merging this pull request may close these issues.

3 participants