_onlyModified is failing

[tjperry07]

I'm testing out the Vale action for onlyModified and I am getting the following failure.

Warning: User-specified path (__onlyModified) is invalid; falling back to 'all'.

You can review my code and test here https://github.com/tjperry07/vale-setup/pull/1/checks?check_run_id=1413681722

Run errata-ai/[email protected]
/usr/bin/docker run --name ad72896c53a74836b0358e95b19b1553_016fa3 --label 179394 --workdir /github/workspace --rm -e GITHUB_TOKEN -e INPUT_FILES -e INPUT_STYLES -e INPUT_CONFIG -e INPUT_DEBUG -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "/home/runner/work/_temp/_github_home":"/github/home" -v "/home/runner/work/_temp/_github_workflow":"/github/workflow" -v "/home/runner/work/_temp/_runner_file_commands":"/github/file_commands" -v "/home/runner/work/vale-setup/vale-setup":"/github/workspace" 179394:ad72896c53a74836b0358e95b19b1553  "" "" "__onlyModified" "false"
Warning: User-specified path (__onlyModified) is invalid; falling back to 'all'.
Vale: Found 0 suggestion(s), 0 warning(s), and 4 error(s).

action file

name: Linting
on: [push]

jobs:
  prose:
    runs-on: ubuntu-latest
    steps:
    - name: Checkout
      uses: actions/checkout@master

    - name: Vale
      uses: errata-ai/[email protected]
      with:
        # Optional
        # styles: |
        #   https://github.com/errata-ai/Microsoft/releases/latest/download/Microsoft.zip
        #   https://github.com/errata-ai/write-good/releases/latest/download/write-good.zip

        # # Optional
        # config: https://raw.githubusercontent.com/errata-ai/vale/master/.vale.ini

        # Optional
        files: __onlyModified
      env:
        # Required
        GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

[tjperry07]

It also returns an error for onlyAnnotateModifiedLines
Warning: Unexpected input(s) 'onlyAnnotateModifiedLines', valid inputs are ['entryPoint', 'args', 'styles', 'config', 'files', 'debug']

[jdkato]

These are unreleased features still, so you need to use the @master version (not @v1.3.0) to get access to them.

[jouni]

I tried using @master but got a slightly different error:

Error: Command failed with exit code 128: git diff --name-only undefined

I mentioned it here: #22 (comment)

[tjperry07]

I got the same error as @jouni when using @master.

This works to check only modified files. It uses another action to grab only changed files and then vale checks that file. This works until _onlyModified is working.

name: Linting
on: [push]

jobs:
  prose:
    runs-on: ubuntu-latest
    steps:
    - name: Checkout
      uses: actions/checkout@master
    - name: Get Changed Files
      id: get_changed_files
      uses: lots0logs/[email protected]
      with:
        token: ${{ secrets.GITHUB_TOKEN }}
    - name: Vale
      uses: errata-ai/[email protected]
      with:
        files: '${{ steps.get_changed_files.outputs.all }}'
      env:
        # Required
        GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

[tcmetzger]

Just raising my hand to support adding onlyAnnotateModifiedLines to this great action. And thanks to @tjperry07 for the workaround!

[robb-romans]

Hi, looking forward to using this flag. Any news on a fix? Thanks.

[jdkato]

Getting a new release out is on my to-do list, but I don't have a concrete timeline.

[jdkato]

@pierrebeitz do you have any thoughts here?

My inclination is to move away from using raw Git commands in favor of using GitHub's API (/compare/ and pull-requests-files). I think this will also allow us to avoid having to call ensureGitHistory.

[pierrebeitz]

@jdkato at first sight that sounds very reasonable!
I plan to have a deeper look at this after Jan 4 (in case the issue persists till then).

[jdkato]

This should be fixed on @master now. Pending more testing, I'll make a new release.

[jouni]

To report back: I just updated our workflow, and it seems to be working nicely. Notably, I changed from [push] to [pull_request], and now the workflow checks all files within a PR, not just the ones within one commit.

Here’s our config:

name: Vale
on: [pull_request]
jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - name: checkout
        uses: actions/checkout@master
      - name: vale
        uses: errata-ai/vale-action@master
        with:
          styles: |
            https://github.com/errata-ai/Microsoft/releases/latest/download/Microsoft.zip
            https://github.com/errata-ai/Google/releases/latest/download/Google.zip
            https://github.com/errata-ai/write-good/releases/latest/download/write-good.zip
          config: https://raw.githubusercontent.com/vaadin/docs/master/.vale.ini
          files: __onlyModified
        env:
          GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}

[jouni]

One issue I’m noticing now is that if a file is removed during a PR review, it can cause the action to fail with the following:

That particular file ended up being renamed during the PR.

[jdkato]

@jouni: I think that should be fixed now.

[jouni]

What about onlyAnnotateModifiedLines, is that supposed to work already?

[jdkato]

Yes, I think so -- but testing the action adequately has proven difficult, so YMMV.

[jouni]

I’m testing it in our docs, and I’m getting inconsistent results. See vaadin/docs#144

Some checks are annotated correctly, but some are missing (for example heading case, we require using Title Case).

The result varies between runs: 7 annotations after a couple of clean runs, and the only 4 annotations after making an edit that should add one more error.

[jouni]

Any advice here? It’s a bit problematic for us at the moment. We get a lot of noise from the existing errors/warnings in our content, and it’s hard to focus on the modified lines.

We also sometimes get false positives, and we have Vale as a required check in PRs, so that prevents non-admins from merging. I hope this is a problem in our config/styles (instead bugs in Vale), but I haven’t been able to fix it.

[jdkato]

There seems to be a lot of different topics being discussed here.

1. Looking specifically at Vale: test onlyAnnotateModifiedLines vaadin/docs#144, it appears to me that the action is working correctly: the annotation counts are different between commits because the changed lines are different.

   For example, 0c5ad68 has 5 errors including a misspelling of "compelting" on line 11. 2789506 (the next run) doesn't include the errors from 0c5ad68 (such as "compelting") because those lines weren't changed in the current commit.

2. It's not totally clear to me where the "Unchanged files with check annotations" section is coming from, or if it can be disabled. "Unchanged files with check annotations" is disruptive and should be configurable actions/toolkit#457 may be related.

We also sometimes get false positives, and we have Vale as a required check in PRs, so that prevents non-admins from merging. I hope this is a problem in our config/styles (instead bugs in Vale), but I haven’t been able to fix it.

I won't be able to help here without specific examples. Running Vale from the command line (locally) should provide an indication of whether the issue is with Vale itself or the action.

[jouni]

There seems to be a lot of different topics being discussed here.

Sorry about that. I don’t want to be noisy, but I’m struggling a bit to understand all the issues together. I’ll try to stick to the issue about annotating the correct lines in a PR.

---

Looking specifically at vaadin/docs#144, it appears to me that the action is working correctly: the annotation counts are different between commits because the changed lines are different.

Perhaps I’ve then misunderstood how the action should work with PRs. I understood that it should always annotate all changes within one PR, not just for the last commit in the PR. My mistake then. But I feel the current behavior is problematic, as it means we can’t rely on Vale as a required check for PRs. It means that if the last commit in a PR branch doesn’t contain any Vale errors, any errors in the previous commits can go unnoticed (when viewing changes from all commits, only one of the files has annotations).

2789506 (the next run) doesn't include the errors from 0c5ad68 (such as "compelting") because those lines weren't changed in the current commit.

If that’s intended behavior, then I understand it. But what I don’t understand is why Vale didn’t give an error for the changed line there, where the heading is changed from “Required Tools” to “Required Tools”. We require title case, see our check for that: Headings.yml. Maybe that’s some issue in our config, as it didn’t work in another commit (vaadin/docs@85d8aca) either (update: tried locally, and I do get an error for that line)

[robb-romans]

The most useful outcome would be for Vale to run against all changes in a PR, since the basis for approval of a PR is the set of all changes in the PR.

[jdkato]

It's not just the last commit; it's every commit individually.

It seems like you'd like it to be every commit collectively. In other words,
every commit would also include all annotations from all previous commits.

This is possible, but less straightforward since the it only applies to PRs (whereas the the current functionality can apply to pushes too) and the content of a line can change between commits.

For now, I think just using files: __onlyModified is closer to what you want. But we can discuss this more, although I think I'll move it to a new issue.

[pierrebeitz]

sorry to chime in late again, but lockdown along with close child cares is just eating up time :)

i initially implemented __onlyModified with the intention of it to work on PRs, as that's exactly the use case we need at work. just to add a data point to the discussion. unfortunately i can not promise to work on this anytime soon.

@jouni, have you tried 0d66d6f? that seems to work for us at work and should give feedback on whole PRs and not single commits. does not work for "push" though as outlined by @jdkato.

[jouni]

@pierrebeitz, thanks for the suggestion (and thanks for that contribution 👍 ).

We are using vale-action@master at the moment, with files: __onlyModified (but without onlyAnnotateModifiedLines), and it’s working alright. We just have a lot "debt" from before, so every PR gets a lot of noise from unmodified lines 😅

[jouni]

Thanks again to @jdkato for being very responsive 🙇

[pierrebeitz]

Oh, only now I realize that I meant onlyAnnotateModifiedLines. Sorry for the noise and false info.

[prcr]

@pierrebeitz, thanks for the suggestion (and thanks for that contribution ).

We are using vale-action@master at the moment, with files: __onlyModified (but without onlyAnnotateModifiedLines), and it’s working alright. We just have a lot "debt" from before, so every PR gets a lot of noise from unmodified lines

I'm using this same configuration, and find that this setup encourages gradually solving existing issues in the files that are being changed. Since you already have some context about the information on that file, normally it's not a big extra effort to review the identified issues (this may be different if you have very big files though).