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

Modify expect-error input checking to fix errors on main #1190

Merged
merged 3 commits into from Aug 17, 2022

Conversation

henrymercer
Copy link
Contributor

We're seeing some errors in main from the function that ensures that expect-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 to true. 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

  • 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.

This should be more robust than determining whether the repo is the
CodeQL Action or a fork of it.
@henrymercer henrymercer requested a review from a team as a code owner August 17, 2022 13:50
// 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}`;
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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 👍

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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!

@henrymercer henrymercer merged commit 3154c4f into main Aug 17, 2022
@henrymercer henrymercer deleted the henrymercer/fix-debug-artifact-tests-on-push branch August 17, 2022 14:49
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