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

feat: add inline-ignore #375

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

kounoike
Copy link

@kounoike kounoike commented Nov 1, 2023

add inline-ignore feature.
this resolves #237

@vl-kp
Copy link

vl-kp commented Feb 8, 2024

someone please merge this PR

@rhysd
Copy link
Owner

rhysd commented Feb 10, 2024

This PR is not acceptable because

  • no test
  • no document
  • it cannot ignore errors in the middle of a multi-line string like below
    # How can we ignore the error from the unknown variable `hoge`?
    - run: |
        ...
        ...
        ${{ hoge }}

The feature requested at #237 is more complicated than it looks actually.

@kounoike
Copy link
Author

@rhysd Could you merge this PR?

  • I wrote test
  • I wrote document
  • it can ignore error in multi-line string
        # actionlint ignore=potentially untrusted
        run: |
          echo "hello"
          echo '${{ github.event.head_commit.author.name }}'
          echo "hello"

I hope to have a constructive discussion.

@rhysd
Copy link
Owner

rhysd commented Mar 22, 2024

Thank you for addressing some of my comments.

it can ignore error in multi-line string

That's possible because the error position is poor.

I'm sorry that I was not able to describe my concern clearly.

For example,

L10 - run: |
L11    ...
L12    ...
L13    ${{ hoge }}

When an error is found at L13, actionlint reports an error happened at L10. So implementation in this branch somehow currently works fine and the ignore comment can remove the error.

However, when we improve the error position in the future and actionlint can report an error happened at L13, then this implementation will break. The ignore comment above L10 cannot remove an error at L13. And there is no possible fix in that case.

So this implementation happens to work correctly for now, but it's depending on poor error location. Once the poor error location issue is solved, this implementation no longer works.

This is what I concern.

@ChrisCarini
Copy link
Contributor

@rhysd I was wondering about the 'poor error location' when I've been using this tool - is there a separate issue tracking that bug/improvement? I'd certainly love to see this improve.

My 2 cents on this PR, if this works w/ the current way the error location is reported, I think this is fine, and when the poor error location problem gets addressed, the implementation in this change can be updated to accommodate it as well - what do you think?

@thiagowfx
Copy link

thiagowfx commented Aug 21, 2024

Is there another way to disable checks in the meantime?

For example, I got a false positive due to github composite actions:

.github/workflows/foo.yml:31:31: property "test_server_ip" is not defined in object type {} [expression]

The input exists in the composite action but actionlint cannot recognize it (#401). I cannot seem to find a way to disable the check in the meantime.

Edit: This turned out to be an issue on my end, never mind.

@tbutler-qontigo
Copy link

@rhysd I was wondering about the 'poor error location' when I've been using this tool - is there a separate issue tracking that bug/improvement? I'd certainly love to see this improve.

My 2 cents on this PR, if this works w/ the current way the error location is reported, I think this is fine, and when the poor error location problem gets addressed, the implementation in this change can be updated to accommodate it as well - what do you think?

I would agree with this - the feature would be useful today but you are rejecting the PR on the basis of a change that may or may not happen in the future. Given that the poor error location has not yet been addressed, it would seem to be tricky for the author to change his PR to work with this unwritten future code.

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.

Inline ignores in files
6 participants