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

Stop setting CODEQL_RUNNER environment variable if CLI already sets it #2081

Merged
merged 4 commits into from Jan 12, 2024

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Jan 11, 2024

We are now setting the CODEQL_RUNNER environment variable in the codeql database init step (see backlinked internal PR), so for versions of the CLI that support this feature, we can stop setting it in the Action.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@angelapwen
Copy link
Contributor Author

Hm...

Deleting failed SARIF upload
  In test mode, therefore deleting the failed analysis to avoid impacting tool status for the Action repository. SARIF ID to delete: 3146eec2-b0d6-11ee-9ed6-a261d13cdc7d.
  Analysis ID to delete: 160977804.
Error: Failed to delete uploaded SARIF analysis. Reason: Error: HttpError: Resource not accessible by integration

Odd as we have not changed anything about our token permissions between this PR and the last run. Also, our PUT calls to the analysis and analysis/status endpoints are working, so we should still have the necessary security_events:write permissions for a DELETE (https://docs.github.com/en/rest/code-scanning/code-scanning?apiVersion=2022-11-28#delete-a-code-scanning-analysis-from-a-repository)

@angelapwen angelapwen marked this pull request as ready for review January 11, 2024 23:42
@angelapwen angelapwen requested a review from a team as a code owner January 11, 2024 23:42
@aeisenberg
Copy link
Contributor

aeisenberg commented Jan 12, 2024

That is odd. Maybe try re-running just in case that was a fluke. The workflow has security-events: write

@angelapwen
Copy link
Contributor Author

I had re-run it on a previous commit too, probably have 5 total consecutive runs failing now 🤔 might be some kind of API error but odd that it's not hit on any of the other times we're hitting code scanning endpoints.

@henrymercer
Copy link
Contributor

I think the "Submit SARIF after failure" check doesn't work on forks — we need to update it so it doesn't try to delete the SARIF when running from a fork. I've backlinked an internal issue.

@aeisenberg
Copy link
Contributor

Right... I forgot about that.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

LGTM

@angelapwen
Copy link
Contributor Author

I think the "Submit SARIF after failure" check doesn't work on forks — we need to update it so it doesn't try to delete the SARIF when running from a fork. I've backlinked an internal issue.

Oh shoot I also forgot about that. Thanks!

@angelapwen
Copy link
Contributor Author

I'm going to remove it from being Required until we have that backlinked issue done.

@angelapwen angelapwen merged commit 9653106 into github:main Jan 12, 2024
627 of 633 checks passed
@angelapwen angelapwen deleted the runner-env-var-feature branch January 12, 2024 17:41
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

3 participants