Skip to content

Exclude specific issues during shellcheck scan (closes #22)#39

Closed
asadmansr wants to merge 4 commits intodevelopfrom
feature/add-shellcheck-exclude
Closed

Exclude specific issues during shellcheck scan (closes #22)#39
asadmansr wants to merge 4 commits intodevelopfrom
feature/add-shellcheck-exclude

Conversation

@asadmansr
Copy link
Contributor

@asadmansr asadmansr commented Oct 23, 2021

Pull Request Checklist

  • I have created an issue prior to creating this pull request
  • I have provided a detailed description and motivation regarding the change below
  • I have updated the test suite and documentation to support my change

Corresponding Issue

Issue already attached.

Description of Change

This features allows the shell-linter github action to exclude specific issues when running the ShellCheck linter. This is useful for accepting lint issues that are not required to be fixed at the moment or at all depending on the requirement.

Motivation and Context

Enhances the feature set of Shell-Linter and also a feature request by an user.

Testing Steps

Added test cases to support the development

Risks

N/A

Additional Information

Updated the README on how users can apply these changes to their projects.

@asadmansr asadmansr changed the title Exclude specific issues during shellcheck scan Exclude specific issues during shellcheck scan (closes #22) Oct 23, 2021
@asadmansr asadmansr linked an issue Oct 23, 2021 that may be closed by this pull request
@asadmansr asadmansr requested a review from Azbagheri October 27, 2021 01:52
entrypoint.sh Outdated
fi

optional_params=""
if [[ ! -z "$exclude_code" ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

we can use -n instead of ! -z

@@ -0,0 +1,6 @@
#! /bin/bash
Copy link
Owner

Choose a reason for hiding this comment

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

how about renaming the folder to exclude_issues to be more descriptive.

@@ -0,0 +1,41 @@
#! /bin/bash

source ./entrypoint.sh "" "" "" "--test"
Copy link
Owner

Choose a reason for hiding this comment

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

same here. Lets rename the test file to exclude_issues_tests.sh

local expected_second_error="SC1068"
local actual_message=$(process_input)

assertContains "Did not find the message." "$actual_message" "$expected_first_error"
Copy link
Owner

Choose a reason for hiding this comment

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

To assure that the SC2039 code is excluded, we should also add an assertion like assertNotContains to check that the actual_message doesn't contain the excluded_code.

test_exclude_multiple_errors(){
input_paths="./test_data/exclude/test_script_exclude_multiple_errors.sh"
severity_mode="style"
exclude_code="SC2034,SC2005,SC2034,SC1066"
Copy link
Owner

Choose a reason for hiding this comment

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

SC2034 is listed twice

assertContains "Did not find the message." "$actual_message" "$expected_message"
}

tearDown(){
Copy link
Owner

Choose a reason for hiding this comment

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

not sure if we need this tearDown() function here

README.md Outdated

Optional. Specify minimum severity of errors to consider [style, info, warning, error]. Default: `style`

### `exclude`
Copy link
Owner

Choose a reason for hiding this comment

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

Since we are adding another input for excluding files and folders, we need to be more specific in naming. How about using exclude_issues?

YueMiyuki and others added 2 commits February 10, 2022 07:04
Fix error `every step must define a `uses` or `run` key`

Co-authored-by: Justin Watts <justin@azohra.com>
@asadmansr asadmansr force-pushed the feature/add-shellcheck-exclude branch from 1b40ba5 to cba41f1 Compare March 8, 2022 15:51
@Azbagheri
Copy link
Owner

Closing this PR as the more up to date branch is already merged #63

@Azbagheri Azbagheri closed this Feb 5, 2025
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.

Allow to exclude types of warnings

3 participants