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

New GitHub Code Scanning feature makes unwanted comments on each PR #215

Assignees
Labels

Comments

@jamacku
Copy link
Member

jamacku commented Mar 18, 2023

Type of issue

Bug Report

Description

New GitHub feature announced via blog - Code scanning shows more accurate and relevant alerts on pull requests

image

Example:

Describe the solution you'd like

We should probably check what GitHub means by "The change ensures code scanning only shows accurate and relevant alerts for the pull request" and by "Now, the tool reports only alerts inside the lines of code that the pull request has changed, which makes it easier to fix these contextualized alerts in a timely manner." Is GitHub doing differential reports now?

Maybe using the official way how to upload SARIF will solve our issue: https://github.com/github/codeql-action/blob/v2/upload-sarif/action.yml

@jamacku jamacku self-assigned this Mar 18, 2023
@evverx
Copy link

evverx commented Mar 18, 2023

Maybe using the official way how to upload SARIF will solve our issue

I think it should probably help but looking https://github.com/search?q=%22As+part+of+the+setup+process%2C+we+have+scanned+this+repository+and+found+no+existing+alerts.%22&type=issues it seems GitHub just broke actions that don't look like CodeQL to cater to CodeQL and that wasn't very nice probably.

@evverx
Copy link

evverx commented Mar 18, 2023

I think it could be that GH shows this because there are no shellcheck alerts in the systemd repository and because of that GH assumes that the action has just been introduced. If that is the case it's probably a GH bug.

@jamacku
Copy link
Member Author

jamacku commented Mar 18, 2023

Yes, It seems like a GH bug. Im going to do some testing to see if I can find some workaround. Thanks for looking into this @evverx

@jamacku
Copy link
Member Author

jamacku commented Mar 22, 2023

Ok, so I think the way how to fix it, at least for the systemd repo, is to trigger workflow on push as well.

current config: - jamacku/systemd#72
with on: push: - jamacku/systemd#74

I'll prepare PR.

@jamacku jamacku pinned this issue Mar 22, 2023
YHNdnzj pushed a commit to YHNdnzj/sbupdate that referenced this issue Mar 25, 2023
`differential-shellcheck@v4` added support for `on: push` triggers.

Related to:
- redhat-plumbers-in-action/differential-shellcheck#215
YHNdnzj added a commit to YHNdnzj/arch-install-scripts that referenced this issue Mar 26, 2023
mrc0mmand added a commit to mrc0mmand/mkosi that referenced this issue Mar 28, 2023
to get rid of the annoying message on new PRs.

Also, update the action to the latest version and drop now unnecessary
permissions.

See: redhat-plumbers-in-action/differential-shellcheck#215
Addresses: systemd#1413 (comment)
behrmann pushed a commit to systemd/mkosi that referenced this issue Mar 28, 2023
to get rid of the annoying message on new PRs.

Also, update the action to the latest version and drop now unnecessary
permissions.

See: redhat-plumbers-in-action/differential-shellcheck#215
Addresses: #1413 (comment)
zmiklank pushed a commit to sclorg/container-common-scripts that referenced this issue Mar 30, 2023
jamacku added a commit to jamacku/systemd-rhel9 that referenced this issue Mar 30, 2023
jamacku added a commit to jamacku/systemd-rhel8 that referenced this issue Mar 30, 2023
@praiskup
Copy link

praiskup commented Apr 1, 2023

Ok, so I think the way how to fix it, at least for the systemd repo, is to trigger workflow on push as well.

Is this a workaround, or a real fix? Has this been reported as GitHub issue?

@evverx
Copy link

evverx commented Apr 1, 2023

For that new GH feature to work it has to be able to compare PRs with the "main" branch so I'd say it's a real fix in the sense that it makes the action compatible with that feature. To get rid of those annoying comments GH would have to somehow special case actions that run on PRs only and I don't think GH is particularly interested in it (because it would most likely involve partially rolling back that feature or allowing certain actions to opt-out). I don't know if it has been reported or not but looking at the number of the comments I think GH is aware of them.

praiskup added a commit to praiskup/flask-whooshee that referenced this issue Apr 4, 2023
praiskup added a commit to praiskup/flask-whooshee that referenced this issue Apr 4, 2023
praiskup added a commit to fedora-copr/flask-whooshee that referenced this issue Apr 4, 2023
praiskup added a commit to praiskup/vcs-diff-lint-action that referenced this issue Apr 4, 2023
We have to specify the 'push' target.  This avoids the message:

| You have successfully added a new vcs-diff-lint configuration
| .github/workflows/python-diff-lint.yml:python-lint-job. As part of the
| setup process, we have scanned this repository and found no existing
| alerts. In the future, you will see all code scanning alerts on the
| repository Security tab.

... in every single PR.  Without 'push' defined GitHub thinks that
the linter action is added in every single pull-request.

While on it, move the 'if' statements down so the rules are slightly
more readable.

Related: redhat-plumbers-in-action/differential-shellcheck#215
praiskup added a commit to praiskup/vcs-diff-lint-action that referenced this issue Apr 4, 2023
We have to specify the 'push' target.  This avoids the message:

| You have successfully added a new vcs-diff-lint configuration
| .github/workflows/python-diff-lint.yml:python-lint-job. As part of the
| setup process, we have scanned this repository and found no existing
| alerts. In the future, you will see all code scanning alerts on the
| repository Security tab.

... in every single PR.  Without 'push' defined GitHub thinks that
the linter action is added in every single pull-request.

While on it, move the 'if' statements down so the rules are slightly
more readable.

Related: redhat-plumbers-in-action/differential-shellcheck#215
praiskup added a commit to praiskup/copr that referenced this issue Apr 4, 2023
It looked like:

    You have successfully added a new vcs-diff-lint configuration
    .github/workflows/python-diff-lint.yml:python-lint-job. As part of
    the setup process, we have scanned this repository and found no
    existing alerts. In the future, you will see all code scanning
    alerts on the repository Security tab.

The reason is that GitHub assumed that every single pull-request
actually *newly* enabled the vcs-diff-lint action.  When we define also
the 'push' action, the assumption no longer applies (sorta bug, not
sure).

Relates: fedora-copr/vcs-diff-lint-action#16
Relates: redhat-plumbers-in-action/differential-shellcheck#215
praiskup added a commit to praiskup/copr that referenced this issue Apr 4, 2023
It looked like:

    You have successfully added a new vcs-diff-lint configuration
    .github/workflows/python-diff-lint.yml:python-lint-job. As part of
    the setup process, we have scanned this repository and found no
    existing alerts. In the future, you will see all code scanning
    alerts on the repository Security tab.

The reason was that GitHub assumed that every single pull-request
actually *newly* enabled the vcs-diff-lint action.  When we define also
the 'push' action, the assumption no longer applies (sorta GitHub bug,
not sure).

Relates: fedora-copr/vcs-diff-lint-action#16
Relates: redhat-plumbers-in-action/differential-shellcheck#215
praiskup added a commit to praiskup/vcs-diff-lint-action that referenced this issue Apr 5, 2023
We have to specify the 'push' target to avoid the message

| You have successfully added a new vcs-diff-lint configuration
| .github/workflows/python-diff-lint.yml:python-lint-job. As part of the
| setup process, we have scanned this repository and found no existing
| alerts. In the future, you will see all code scanning alerts on the
| repository Security tab.

... appearing in every single PR.  Without the 'push' trigger defined,
GitHub thinks that the linter action is being added in every single
pull-request.

Though, since the workflow is newly run for 'push' events, we have to do
some reasonable "no-op".  For the first implementation, we simply try to
generate an empty list of issues (therefore the container fix here).

For the vcs-diff-lint case, it doesn't make too much sense to do the
analysis on pushes.  At least not in the "basic" differential use-case
we have now.  We could e.g. do full-scan in the future if there was
demand for it.

So I'm fixing the example in README.md to define "push", too.  While on
it, move the 'if' statements down so the rules are slightly more
readable.

Related: redhat-plumbers-in-action/differential-shellcheck#215
Closes: fedora-copr#17
Closes: fedora-copr#16
praiskup added a commit to fedora-copr/vcs-diff-lint-action that referenced this issue Apr 5, 2023
We have to specify the 'push' target to avoid the message

| You have successfully added a new vcs-diff-lint configuration
| .github/workflows/python-diff-lint.yml:python-lint-job. As part of the
| setup process, we have scanned this repository and found no existing
| alerts. In the future, you will see all code scanning alerts on the
| repository Security tab.

... appearing in every single PR.  Without the 'push' trigger defined,
GitHub thinks that the linter action is being added in every single
pull-request.

Though, since the workflow is newly run for 'push' events, we have to do
some reasonable "no-op".  For the first implementation, we simply try to
generate an empty list of issues (therefore the container fix here).

For the vcs-diff-lint case, it doesn't make too much sense to do the
analysis on pushes.  At least not in the "basic" differential use-case
we have now.  We could e.g. do full-scan in the future if there was
demand for it.

So I'm fixing the example in README.md to define "push", too.  While on
it, move the 'if' statements down so the rules are slightly more
readable.

Related: redhat-plumbers-in-action/differential-shellcheck#215
Closes: #17
Closes: #16
praiskup added a commit to praiskup/copr that referenced this issue Apr 5, 2023
It looked like:

    You have successfully added a new vcs-diff-lint configuration
    .github/workflows/python-diff-lint.yml:python-lint-job. As part of
    the setup process, we have scanned this repository and found no
    existing alerts. In the future, you will see all code scanning
    alerts on the repository Security tab.

The reason was that GitHub assumed that every single pull-request
actually *newly* enabled the vcs-diff-lint action.  When we define also
the 'push' action, the assumption no longer applies (sorta GitHub bug,
not sure).

Relates: fedora-copr/vcs-diff-lint-action#16
Relates: redhat-plumbers-in-action/differential-shellcheck#215
FrostyX pushed a commit to fedora-copr/copr that referenced this issue Apr 6, 2023
It looked like:

    You have successfully added a new vcs-diff-lint configuration
    .github/workflows/python-diff-lint.yml:python-lint-job. As part of
    the setup process, we have scanned this repository and found no
    existing alerts. In the future, you will see all code scanning
    alerts on the repository Security tab.

The reason was that GitHub assumed that every single pull-request
actually *newly* enabled the vcs-diff-lint action.  When we define also
the 'push' action, the assumption no longer applies (sorta GitHub bug,
not sure).

Relates: fedora-copr/vcs-diff-lint-action#16
Relates: redhat-plumbers-in-action/differential-shellcheck#215
systemd-rhel-bot pushed a commit to redhat-plumbers/systemd-rhel9 that referenced this issue Apr 19, 2023
@jamacku
Copy link
Member Author

jamacku commented Apr 26, 2023

@evverx, @praiskup FYI, It seems like GitHub changed the way how it detects the introduction of new scanning tools, and it no longer requires running ShellCheck/PyLint on a push event.

@evverx
Copy link

evverx commented Apr 26, 2023

@jamacku good to know. Thanks! I think actions still should be triggered on push events to get diffs. My guess would be that without that all alerts would be "new" and invisible for external contributors.

phracek pushed a commit to hhorak/container-common-scripts that referenced this issue May 17, 2023
praiskup added a commit to praiskup/resalloc that referenced this issue Aug 2, 2023
It looked like:

    You have successfully added a new vcs-diff-lint configuration
    .github/workflows/python-diff-lint.yml:python-lint-job. As part of
    the setup process, we have scanned this repository and found no
    existing alerts. In the future, you will see all code scanning
    alerts on the repository Security tab.

The reason was that GitHub assumed that every single pull-request
actually *newly* enabled the vcs-diff-lint action.  When we define also
the 'push' action, the assumption no longer applies (sorta GitHub bug,
not sure).

Relates: fedora-copr/vcs-diff-lint-action#16
Relates: redhat-plumbers-in-action/differential-shellcheck#215
@jamacku jamacku unpinned this issue Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment