Open ejprismfly opened 1 year ago
Seeing the same. Based on the docs it looks like that functionality might be based on the token and base being defined, but I have those set up and not seeing any "annotations".
The "Theme Check Report" table lists all the errors (both failing and non failing) but those don't seem to get converted to annotations that appear on line items in the PR
Based on the docs it looks like that functionality might be based on the token and base being defined.
Exactly. There's two behaviours:
base
is defined and only linting errors on "diffs" appearbase
is undefined and all errors are reportedNote that for annotations to appear on your PR, the errors have to show up on line diffs from your PR. It's common during setup that this doesn't happen because the only content of your PR is the github action itself.
@charlespwd
So to be clear if 'base' is undefined all error are reported (but only appear as annotations if the PR diff includes those lines changed)?
In my case I opened a test PR targeting the branch that introduces this action, and left base undefined. The test PR intentional breaks some white space. I see the warning in theme check report for this rule violation (warning not error) and I see the same line in the diff, yet no annotation. I can provide screenshots if it helps, or perhaps try to repro in a public repo.
Another factor of my config is I am setting a flag so that theme-check doesn't return an error on warnings or suggestions, just errors, however looking how the annotations work it seems to support reporting warnings too and should require an error to be present to work.
Hmm. Looking at index.ts and the only thing I could think of is a root problem?
:rubber-duck: Thoughts:
https://github.com/Shopify/theme-check-action/blob/main/index.ts#L82 seems to say that if there's no /tmp/diff.log (because there's no base), then the filter becomes () => true (always passes).
You're saying that the errors appear in the report but not the annotations? So... the error count would indicate that https://github.com/Shopify/theme-check-action/blob/main/index.ts#L146-L155 is working...
You're not seeing annotations, so https://github.com/Shopify/theme-check-action/blob/main/index.ts#L201-L214 isn't.
My guess right now is a path mismatch problem.
I can provide screenshots if it helps, or perhaps try to repro in a public repo.
Yeah as much as you can that'd help. Config. Report. Logs of runs. As much as you can provide. Don't need the code itself.
@charlespwd I thought it would be super quick to fork Dawn and attempt a repro:
1) forked dawn 2) removed lighthouse action because it requires secrets and is not relevant 3) removed CLA agreement action
However I ran into some trouble
1) It looks like actions/checkout@v2 is deprecated (got node 12 warning you can see here). I did upgrade to v3, not sure if it is impacting
2) even with that warning fixed by upgrading i'm getting a Error: Resource not accessible by integration
after Creating GitHub check...
step.
I've never seen that error before and it seems to be relatively unhandled. The action config is very simple on my main branch:
name: CI
on: [push]
jobs:
theme-check:
name: Theme Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Theme Check
uses: shopify/theme-check-action@v1
with:
token: ${{ github.token }}
I wonder if you have any hints of how to diagnose this issue? Clearly things are working in the dawn repo, I wonder if some permissions work differently on a fork?
So it seems like the github.token isn't enough permissions. How odd. It seems to be failing here:
https://github.com/Shopify/theme-check-action/blob/main/index.js#L84-L90
Looking at https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28, it looks like that API is now for GitHub apps only :thinking_face:?
@charlespwd OK so for the github.token issue is that related to this other issue? https://github.com/Shopify/theme-check-action/issues/13 ? Should we create a new issue specifically for that? Is there a clear path forward to work with the new API version?
Not familiar with octokit however the version of octokit/plugin-throttling used in this action is 3.5.2 which is about two years old. Current version is 5.0.1. I'm guessing it's not a super simple upgrade to from 3.x to 5.x
@charlespwd any thoughts on the next step here?
@charlespwd hopeful nudge on this. Anything you can share to point me in the right direction would be helpful 🙏
Any update? It seems we are facing the same issue @alibosworth @charlespwd - the action fails if there is nothing to do
@charlespwd @ejprismfly @MauriceArikoglu I think I figured out a fix for this, though I don't totally understand the details of why it works.
There is a discussion of a similar "Error: Resource not accessible by integration" on this issue https://github.com/actions/first-interaction/issues/10 so i tried out scoping all permissions as write
, and it worked, then i removed everything one by one until finding that checks: write
was enough to make it work. Here you can see the demo repo i made above, but working: https://github.com/alibosworth/dawn-ci-test/pull/2/files
My YML file looks like this:
name: CI
on: [push]
permissions:
checks: write
jobs:
theme-check:
name: Theme Check
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Theme Check
uses: shopify/theme-check-action@v1
with:
token: ${{ github.token }}
Note: that the above is the fix for the minimal reproduction of the issue however you probably want read
permissions for other scopes, and as soon as you define any scope all undefined are interpreted as none
, so in practice you probably have to sett a bunch to read
(the default?)
Thanks @alibosworth I will try that and get back to you
@alibosworth the change causes the theme action to report "repository not found"
@alibosworth the change causes the theme action to report "repository not found"
@MauriceArikoglu Well it depends what other read permissions your actions needs. When you set one permission all others become "none" implicitly (rather than default "read". Try this:
permissions:
actions: read
checks: write
contents: read
deployments: read
issues: read
discussions: read
packages: read
pages: read
pull-requests: read
repository-projects: read
security-events: read
statuses: read
@alibosworth the change causes the theme action to report "repository not found"
@MauriceArikoglu Well it depends what other read permissions your actions needs. When you set one permission all others become "none" implicitly (rather than default "read". Try this:
permissions: actions: read checks: write contents: read deployments: read issues: read discussions: read packages: read pages: read pull-requests: read repository-projects: read security-events: read statuses: read
Thanks. Unfortunately with these changes the action runs again fine, but completes still with error status.
I also opened #28 and attached a screenshot there, for reference.
Oh OK I think that might be a different issue, or non issue?. Try branching off that change and actually changing code that introduces an error. as @charlespwd mentioned above:
Note that for annotations to appear on your PR, the errors have to show up on line diffs from your PR. It's common during setup that this doesn't happen because the only content of your PR is the github action itself.
@charlespwd @ejprismfly @MauriceArikoglu @alibosworth This issue resolved for me when I changed the action to run on push
and not on pull_request
.
I am now down to loose my github action minutes as this action is going to run on every commit I push.
Would really help if there might be a better optimized solution.
The result of theme check is not showing, the theme check fails and the job ended.
We are expecting something like this!
Is there any help or guide that we can follow? possibly adding an action to read the result?