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

Enable Differential ShellCheck #2530

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jamacku
Copy link
Contributor

@jamacku jamacku commented Oct 3, 2023

Differential ShellCheck is a GitHub action that performs differential ShellCheck scans on shell scripts changed via PR and reports results directly in PR.

It is able to produce reports in SARIF format. GitHub understands this format and is able to display it nicely as a PR comment, and on the Files Changed tab, please see below.

image

image

Checklist

  • I have tested it locally
  • I have reviewed and updated any documentation if relevant
  • I am providing new code and test(s) for it

This will resolve your issues with cleaning your codebase every time the new ShellCheck comes out: #2501

Differential ShellCheck only reports defects directly related to the PR/commit. So you can focus on defects caused by your changes and deal with the existing defects later.

@github-actions github-actions bot added the github Issues related to .github label Oct 3, 2023
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Collaborator

@LaszloGombos LaszloGombos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if 3 separate commits adds value in this PR.

@jamacku
Copy link
Contributor Author

jamacku commented Oct 30, 2023

I can squash them If you prefer.

@LaszloGombos
Copy link
Collaborator

Once you merge this pull request, the 'Security' tab will show more code scanning analysis results

@jamacku Have you had a chance to test this ? I run a brief test and at first run this will open about 300 code scanning issues based on the existing issues we have. Is this what is expected ?

What do you recommend after this happens ? Just leave the existing issues alone and focus on avoiding adding more issues ? Can you perhaps point to a project where we can see this in action ?

@jamacku
Copy link
Contributor Author

jamacku commented Nov 1, 2023

@jamacku Have you had a chance to test this ? I run a brief test and at first run this will open about 300 code scanning issues based on the existing issues we have. Is this what is expected ?

You are right. There are 285 ShellCheck reports in the Security Dashboard. This is expected. It's for your awareness. You can review them and decide how to deal with them. The security dashboard allows you to set notes to each report and dismiss them.

What do you recommend after this happens ? Just leave the existing issues alone and focus on avoiding adding more issues ? Can you perhaps point to a project where we can see this in action ?

I usually leave the existing issues open and focus on not adding more. But the choice is yours.

The security dashboard is "private" to members of the organization, so you won't see the results of other projects that use differential shellcheck unless you are a member. Reports on PR can see anyone.

For example, the systemd project is keeping shell scripts free from ShellCheck defects - https://github.com/systemd/systemd/blob/main/.github/workflows/differential-shellcheck.yml

We also use it on our RHEL8 and RHEL9 dracut distribution git:

It's beneficial to have differential ShellCheck scans because you can start linting your code base without dealing with current reports first (that saves time, allows you to check new incoming changes, and avoids potential regressions caused by fixing false positives).

@pvalena
Copy link
Contributor

pvalena commented Nov 1, 2023

LGTM.

@LaszloGombos
Copy link
Collaborator

@jamacku It seems the "Dracut Release Bot" pushed an extra commit to this PR. Can you help reverting the 4th commit on this PR ? Thanks !

It performs differential ShellCheck scans and reports results directly on GitHub.

documentation: https://github.com/redhat-plumbers-in-action/differential-shellcheck

Signed-off-by: Jan Macku <[email protected]>
The `sh_checker_comment: true` requires special permissions (`pull-requests: write`).
This permission level could be achieved only on PR from the `dracut` repository. When PR is opened from the fork, it automatically drops to `read` only.

error message:
```
Commenting on the pull request
{
  "message": "Resource not accessible by integration",
  "documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment"
}
```
ShellCheck linting is performed by `differential-shellcheck`.
Copy link

stale bot commented Mar 13, 2024

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

@stale stale bot added the stale communication is stuck label Mar 13, 2024
@LaszloGombos LaszloGombos removed the stale communication is stuck label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github Issues related to .github
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants