-
Notifications
You must be signed in to change notification settings - Fork 350
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
Modify expect-error
input checking to fix errors on main
#1190
Modify expect-error
input checking to fix errors on main
#1190
Conversation
This should help us debug failures on `main` like this https://github.com/github/codeql-action/actions/runs/2875586196.
This should be more robust than determining whether the repo is the CodeQL Action or a fork of it.
// Returns whether workflow kicked off by codeql-action repo itself, | ||
// or a fork of it. | ||
export function isAnalyzingCodeQLActionRepoOrFork(): boolean { | ||
const codeQLActionRepoUrl = `https://api.github.com/repos/${CODEQL_DEFAULT_ACTION_REPOSITORY}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm actually I think the problem was that this should have been https://github.com/${CODEQL_DEFAULT_ACTION_REPOSITORY}
. But I have a mild preference for the new solution since that works with the clone that isn't a fork case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the new solution seems reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this URL at first but it seemed like it was the api.github.com
one when I was testing the PR check locally. I guess it's probably different in main
vs in a PR check?
I think your solution is better in any case as well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh you're totally right — it's api.github.com
on a pull_request
trigger, and github.com
on a push
trigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting.. I guess it would've worked if we'd checked for either. Do you mind pointing me to where you found this in the documentation? (if you found it there ha)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just triggered some Actions to find out — see https://github.com/github/codeql-action/runs/7917327160?check_suite_focus=true where it's github.com
on push
and https://github.com/github/codeql-action/runs/7916384170?check_suite_focus=true where it's api.github.com
on ready_for_review
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep, I see, the Dump GitHub event
step gives it away. Thanks!
We're seeing some errors in
main
from the function that ensures thatexpect-error
is only set to true by the CodeQL Action PR checks, for example this run.There doesn't appear to be enough information in the logs to debug the problem, so I've modified the workflow to dump the contents of the GitHub event, and changed the checking behaviour to instead require the
TEST_MODE
environment variable to be set totrue
. This has the benefit of also working in the case that someone clones the Action repo and pushes it up somewhere else without explicitly forking it.Merge / deployment checklist