Skip to content

Abort CodeQL action if the status cannot be reported - #308

Merged
simon-engledew merged 6 commits into
mainfrom
simon-engledew/fast-fail
Nov 20, 2020
Merged

Abort CodeQL action if the status cannot be reported#308
simon-engledew merged 6 commits into
mainfrom
simon-engledew/fast-fail

Conversation

@simon-engledew

@simon-engledewsimon-engledew commented Nov 17, 2020

Copy link
Copy Markdown

Save action minutes by only running the CodeQL check if code scanning is enabled for the repository.

This reverts some work done in #128

This is nearly a no-op against Enterprise but there is value in not maintaining two separate behaviours.

Enterprise will log a warning and not fail on a 422. This may occur if the bundled version of CodeQL was updated on an old version of Enterprise.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • I have only ever seen the Octokit request raise HTTPErrors rather than return a response with non-200 status codes. I need to check with @robertbrignull if this behaviour is new, or if there is an edge case that I have not managed to reproduce in my local setup.

I think this is likely a change in octokit. As both the response and the HTTP error have a status field it seems reasonable to check it on both success and failure using the same code. If it changes in future this code should continue to work the same.

Put more fine grained logic around which errors we ignore and process.
Re-instate status reporting in Enterprise.
Abort the code scanning process the status endpoint reports it is not configured.
@simon-engledew

simon-engledew commented Nov 18, 2020

Copy link
Copy Markdown
Author

@robertbrignull I had a chat with Marco and he is keen to keep using the status endpoint to determine if code scanning is enabled.

To do so, I have had to undo your previous change and start treating 404 and 403 as stop conditions.

I have attempted to keep to the spirit of the original code by treating other errors in a similar way:

  • 422 - ignore it in Enterprise, it's probably just a mismatched codeql-action<->api due to an upgrade of the action (i.e: broken forward compatibility)
  • !200 - ignore it for initial reporting as it may be transient. do not ignore the error for later reporting.

Unstitching something makes me nervous. I would be very grateful if you could give this a once over and see if it makes sense! ❤️

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally looks good and looks like it should work, though I had a few comments.

Comment threadsrc/actions-util.ts Outdated
Comment threadsrc/actions-util.ts Outdated
Comment threadsrc/actions-util.ts Outdated
Comment threadsrc/actions-util.ts Outdated
@simon-engledew
simon-engledew marked this pull request as ready for review November 19, 2020 12:36
Until there is a more robust versioning system it is probably safest to require endpoint compatiblity and not continue the action if there is a mismatch.
Comment threadsrc/actions-util.ts Outdated

@robertbrignullrobertbrignull left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. The changes LGTM now, and I've tried it out on a GHES instance and it works as expected.

@simon-engledew
simon-engledew merged commit 31872f1 into mainNov 20, 2020
@simon-engledew
simon-engledew deleted the simon-engledew/fast-fail branch November 20, 2020 10:45
@github-actionsgithub-actionsBot mentioned this pull request Nov 23, 2020
Sign up for freeto 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.

2 participants

@simon-engledew@robertbrignull