Skip to content
This repository has been archived by the owner on Jan 14, 2024. It is now read-only.

Use GHA workflow commands in PRs from forks instead of API calls #3

Open
webknjaz opened this issue Jul 22, 2020 · 20 comments
Open

Use GHA workflow commands in PRs from forks instead of API calls #3

webknjaz opened this issue Jul 22, 2020 · 20 comments

Comments

@webknjaz
Copy link

GITHUB_TOKEN in forked repos cannot write to Checks API: https://docs.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token.

I suggest falling back to using Workflow commands (https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-a-warning-message) in order to overcome this limitation.

The idea is simple: just output things like ::warning file={name},line={line},col={col}::{message} to stdout and GitHub Action runner will pick those up and post them as annotations.

@shyim
Copy link
Owner

shyim commented Jul 22, 2020

Thats much better idea. It's also easier 😅

@ssbarnea
Copy link

I am looking forward to see it working as annotations are really awesome. And thanks for the work on that.

Hopefully we can now make it easier for developers to read their own errors.

@webknjaz
Copy link
Author

You should be able to use import this from the actions toolkit: https://github.com/actions/toolkit/blob/b0e01b71c0e630eb4b420f763029a7476c6cf075/packages/core/src/command.ts

import * as core from '@actions/core';
core.issueCommand(
  'warning',
  {
    file: 'PASTE THE FILE PATH HERE',
    line: 42,
    col: 42,
  },
  'YOUR MESSAGE HERE',
)

(https://github.com/actions/toolkit/blob/master/packages/core/src/core.ts#L145)

@webknjaz
Copy link
Author

P.S. I'd still use the API where possible because workflow commands have some character limitations. And while @actions/core escapes some things (actions/toolkit#302), it still breaks on some chars.

shyim added a commit that referenced this issue Jul 22, 2020
@shyim
Copy link
Owner

shyim commented Jul 22, 2020

Hey,
first thanks for your Ideas. I did built it randomly to see my junit logs and didn't expected that somebody else would use it too, thats the reason of the missing README 🙈 .

Do you mean like this?
898426e

Do you guys now how to test this correctly? Last time I did many releases 🙈

@webknjaz
Copy link
Author

first thanks for your Ideas. I did built it randomly to see my junit logs and didn't expected that somebody else would use it too, thats the reason of the missing README .

Ah, honestly, I was going to build an action like this but decided to check for the prior art so that there'll be no unnecessary double effort.

Do you mean like this?
898426e

Something like this but I think it should also use the old way when running for non-fork PRs because of the limitations I mentioned above.

Do you guys now how to test this correctly? Last time I did many releases

Testing around GHA is a long-time mystery. I know that one of the approaches that GitHub suggests is to have a self-testing workflow in the same repo. Some people probably use https://github.com/nektos/act.

You can also test that commit without releasing by just using shyim/junit-report-annotations-action@898426e in some workflow (maybe in a separate repo) and seeing how it works.

@shyim
Copy link
Owner

shyim commented Jul 22, 2020

I have now built it in the master like your description :)

@shyim shyim closed this as completed Jul 22, 2020
@webknjaz
Copy link
Author

@shyim currently, the annotations show up with the wrong file, have you tested this? https://github.com/ansible/ansible-lint/actions/runs/178928109.

Also, col param is not really necessary, I think. Just line should be enough...

@shyim shyim reopened this Jul 22, 2020
@shyim
Copy link
Owner

shyim commented Jul 22, 2020

Yep I have tested it in my branch. I use PHPUnit with jUnit. Maybe the schema is not very similar.

Can you provide me an example file? :)

@webknjaz
Copy link
Author

Here you go: pytest-junit-example.tar.gz.

P.S. It'd be useful to also log the file locations to stdout too for debugging...

@shyim
Copy link
Owner

shyim commented Jul 22, 2020

Thanks i will Look tomorrow into

@shyim
Copy link
Owner

shyim commented Jul 23, 2020

Hey,

all tests are passed in that file 🙈

@ssbarnea
Copy link

Something is wrong I updated stripFromPath: .tox/ and I am 100% one test fails and that the report file is generated on disk. Still, I do not see any output from junit-report-annotations-action execution.

I also configured junit-report-annotations-action@master to force it to use master version. Maybe it would be a good idea to make it print something like which XML files it found, and how many tests where found on them?

@shyim
Copy link
Owner

shyim commented Jul 23, 2020

@webknjaz
Copy link
Author

@shyim btw you could move path default from the js file into action.yml

@webknjaz
Copy link
Author

@ssbarnea that setting modifies the paths that are inside of the report, not the report lookup on disk. Previous versions generated annotations on the workflow page, just not matched to the proper files.

@webknjaz
Copy link
Author

@ssbarnea I've found the root of the issue. We use xunit2 in pytest. xunit1 used to have the file attribute but xunit2 doesn't have it. And so out XML report file does not contain any file attributes, hence there's nowhere to take the paths from:
pytest-dev/pytest@335cc5d#diff-325679eb11eab11fe155f425c27a8599R81

Possible solutions:

  1. explore long overdue pytest-nunit (but parsing it would require a new GHA or extending this one with the new schema)
  2. downgrade to xunit1 (this would probably work)
  3. make a plugin that'd emit workflow annotations commands directly (probably preferable)

@webknjaz
Copy link
Author

@shyim sometimes using stripFromPath causes ##[error]Cannot read property 'replace' of undefined: https://github.com/ansible/ansible-lint/pull/910/checks?check_run_id=904490377#step:15:10

@webknjaz
Copy link
Author

Using xunit1 made it work, I guess it's a good start for us for now.

@webknjaz
Copy link
Author

@shyim there's one problem with the annotations: it looks like there's an off-by-one. Our XML file contains lines with 0-based numbering while diffs on GH start with 1. We probably need an option to tell this action to increment the line number after reading from the report.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants