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

Fix running tests on forks, and handle invalid URIs when fingerprinting #1694

Merged
merged 5 commits into from
May 25, 2023

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented May 23, 2023

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.

@jsoref jsoref requested a review from a team as a code owner May 23, 2023 06:03
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, most of the changes here look good to me. To help us when we release the Action and when we're going through PRs in the future, would you mind giving your PR a more descriptive title, for instance "Fix running tests on forks, and handle invalid URIs when fingerprinting"?

pr-checks/checks/analyze-ref-input.yml Outdated Show resolved Hide resolved
src/init-action-post-helper.ts Outdated Show resolved Hide resolved
src/workflow.ts Outdated Show resolved Hide resolved
src/fingerprints.ts Show resolved Hide resolved
@jsoref jsoref changed the title Fixes Fix running tests on forks, and handle invalid URIs when fingerprinting May 23, 2023
@jsoref
Copy link
Contributor Author

jsoref commented May 23, 2023

One final pain point: rate limits: https://github.com/check-spelling/github-codeql-action/actions/runs/5053027012 ... I'm not quite sure how to address this and thus I've left it out of scope, but it's problem.

src/init-action-post-helper.ts Fixed Show resolved Hide resolved
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution. This LGTM once you've addressed the linter warnings. If you could make sure npm run build && npm test && npm run lint succeeds locally before submitting we should be good to go.

Re rate limits, we have an item on the backlog to reduce the number of concurrent jobs that run with our PR checks. We could also look at skipping certain PR checks when we run on forks, but that leaves a gap in test coverage I'm not entirely happy with.

@jsoref
Copy link
Contributor Author

jsoref commented May 25, 2023

I'll try to do that this morning.

Note that:

  • The read me file doesn't link to Contributing
  • Contributing mentions node 14 which is almost certainly wrong

I really did look for a build file but didn't find one. I was trying to figure out if the project was npm or yarn.

@jsoref
Copy link
Contributor Author

jsoref commented May 25, 2023

Yes, disabling checks in forks is not what I'd want.

You could use a "variable" to specify a portion of the matrix to run for each test. That'd at least result in not starving forks of running capacity and allow users to change which set they're running between runs.

GitHub should allow running workflows to easily check on runner capacity and declaratively wait for capacity before starting a workflow. (This is very far beyond the scope of this PR, but I'm hoping you can send feedback to some internal team.)

It's probably possible to use workflow dispatch and have workflows chain from one to another. Sure it'd mean that things would be slower, but it's really painful to manually discover individual workflows that have failed and need to be rerun and then run then again. Again, this could be gated on a variable to allow the current behavior for people who have the capacity or tolerance.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates and your further suggestions around the contribution documentation and PR checks. The .github/workflows/pr-checks.yml workflow does something similar to the chaining behaviour you suggest — I could see us doing something like that for other checks too.

@henrymercer henrymercer merged commit 1023a08 into github:main May 25, 2023
325 checks passed
@jsoref jsoref deleted the fixes branch May 25, 2023 14:52
@github-actions github-actions bot mentioned this pull request May 25, 2023
6 tasks
@jsoref
Copy link
Contributor Author

jsoref commented May 28, 2023

Fwiw, this change resulted in:
https://github.com/jsoref/examples-testing/actions/runs/5102140844/jobs/9171515485#step:2:1127

Warning: 'this # file ( is 5) evil.txt' is not a valid URI in 'instance.runs[0].results[2].locations[0].physicalLocation.artifactLocation.uri'.

Which is a huge improvement.

I'd love for that to be surfaced, either by a problem-matcher or a ::warning. If you're amenable to either, I can make a PR.

@aeisenberg
Copy link
Contributor

Thanks for the suggestion, but I don't think that would be a good UX. There are some users of the upload-sarif action that rely on generating an invalid URL to signal that a certain file that is expected doesn't exist. See #1703. Using a problem matcher would mean that all users of this action will receive warnings when their workflow finds alerts.

There are things they could do differently to avoid using an invalid URL and we are going to work with them to fix it. But, until then I don't think there should be a warning here.

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.

Error: URI malformed should report the input / offset
3 participants