GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Don't record an overlay status when the job was cancelled by henrymercer · Pull Request #4122 · github/codeql-action Conversation
Contributor
The overlay status mechanism records a marker in the Actions cache when building an overlay-base database fails, so that later runs skip overlay analysis instead of failing the same way, but it records one whenever the analyze step did not report success, which also covers runs that were cancelled, since the Risk assessmentFor internal use only. Please select the risk level of this change:
Which use cases does this change impact?Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsCo-authored-by: Copilot App <[email protected]> Co-authored-by: Copilot App <[email protected]>
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning
Copilot review overview🟢 Approval recommendedThe focused implementation is consistent with existing input handling and covers the new behavior with unit tests. Review tier: Balanced What changed in this PRPrevents cancelled jobs from incorrectly disabling overlay analysis through cached failure markers. Changes:
Files excluded by content exclusion policy (1)
💡 Add a
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
henrymercer
marked this pull request as ready for review
oscarsj
previously approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Uh oh!
There was an error while loading. Please reload this page.
mbg
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Broadly looks good and makes sense, thanks! Just a few considerations that came to my mind while reviewing this.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never nice to have to feed this in through an extra action input, but I don't see a better approach for getting hold of the job status. An alternative option might be to set
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That approach is also more complex, though it might be interesting to explore later, particularly if we also want to evaluate excluding failures from non-CodeQL Action steps.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page. Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+426
to
+429
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we end up with one of these if the workflow job was cancelled? E.g. because it caused a thread abort style exception to be thrown at an inconvenient moment?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I expect that it's possible, but the most common case would be the job genuinely failing in some way. It's worth looking into telemetry more once we've tackled the low-hanging fruit, but I'll leave as is for now.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Co-authored-by: Copilot App <[email protected]> Co-authored-by: Copilot App <[email protected]>
mbg
approved these changes
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes addressing the feedback from my previous review look good, thanks! Agreed on the other two points and happy to leave those for now. We can look into/address them later if necessary.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactionsUh oh!
There was an error while loading. Please reload this page.
This was referenced
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Copilot code review
Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Uh oh!
There was an error while loading. Please reload this page. AssigneesNo one assignedLabels
size/M
ProjectsMilestoneNo milestoneDevelopmentSuccessfully merging this pull request may close these issues. Uh oh!
There was an error while loading. Please reload this page. Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later. | ||||||||||||||||||||||||||||||||||||
| You are visitor 01402503 — GeezerVison does not store the pages you look up. Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required. |