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

Accept command line arguments instead of environment variables #396

Open
mpoberezhniy opened this issue Apr 30, 2024 · 8 comments
Open

Accept command line arguments instead of environment variables #396

mpoberezhniy opened this issue Apr 30, 2024 · 8 comments
Assignees
Labels

Comments

@mpoberezhniy
Copy link
Contributor

Type of issue

Feature Request

Description

Relates to #179
For local use cli args should be accepted.

Describe the solution you'd like

I think index.sh should handle all the scm parameters, pass other -* parameters to shellcheck, and the rest are filenames after -- - pre-commit passes file names to the hook. If the file names are not passed - scan all the files based on INPUT_TRIGGERING_EVENT, BASE and HEAD. I think INPUT_TRIGGERING_EVENT should remain an env variable for the actions only, and manual should be the default mode for local use.

@jamacku jamacku self-assigned this Apr 30, 2024
@jamacku jamacku pinned this issue Apr 30, 2024
@jamacku
Copy link
Member

jamacku commented Apr 30, 2024

Thank you for writing this down. I'll have a look and implement argument parsing.

@mpoberezhniy
Copy link
Contributor Author

mpoberezhniy commented Apr 30, 2024

I took a quick look at the INPUT_INCLUDE_PATH, it duplicates the logic pre-commit has, but it just throws the file names as args. Getting the file names as arguments seems redundant for differential-shellcheck as it is tied to git. What do you think about it?

Upd:
Pre-commit has an option to not pass file names, so the -- logic should be disregarded.

@jamacku
Copy link
Member

jamacku commented Apr 30, 2024

The INPUT_INCLUDE_PATH was introduced for specific use cases when a shell script is not detectable in any other way, so users point to the files manually.

The logic you are proposing seems like it could work the same way. I'll try to create a draft by Friday.

@jamacku
Copy link
Member

jamacku commented Apr 30, 2024

By the way, it is nice to see someone so passionate about the Differential ShellCheck project. I'm just curious what is your motivation :)

If you are a user who wants to implement some specific features or you just like the idea of running Differential ShellCheck scans.

@mpoberezhniy
Copy link
Contributor Author

We usually used Jenkins for linting but upon the growth of our team we found out that it was not convenient for all the members and that's why could not be enforced to be used. We started looking into actions and wanted to implement a whitelisted list of files that should not take part in the quality gates.

But then I found out there is the differential-shellcheck (and the fedora-copr/vcs-diff-lint-action) action that fully exceeded our linting expectations and integrated flawlessly with the GH pull requests.

I have also heard questions on whether it works with other hosting platforms or could be used as hooks, others find the differential linting cool and exciting too :)

I used to contribute to projects I use when I hit a bug or would like to use a feature. But I find differential linting super useful so I would like to contribute as much as I can.

@mpoberezhniy
Copy link
Contributor Author

I was thinking about refactoring action.yaml a little so it's more compatible with an executable differential-shellcheck and cli args. I came up with the following inputs. What do you think?

inputs:
  base:
    description: Hash of base commit.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}${{ github.event.pull_request.base.sha }}${{ else if github.event_name == 'push' }}${{ github.event.before }}${{ endif }}
  head:
    description: Hash of head commit.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}${{ github.event.pull_request.head.sha }}${{ else if github.event_name == 'push' }}${{ github.event.after }}${{ endif }}

  diff-scan:
    description: Input allowing to request specific type of scan.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}'true'${{ else if github.event_name == 'push' }}'false'${{ endif }}
  strict-check-on-push:
...

@jamacku
Copy link
Member

jamacku commented May 3, 2024

But then I found out there is the differential-shellcheck (and the fedora-copr/vcs-diff-lint-action) action that fully exceeded our linting expectations and integrated flawlessly with the GH pull requests.

Both differential-shellcheck and vcs-diff-lint-action internally use csdiff for scan compression. Speaking of differential scanning, you might be interested in OpenScanHub. It's a service that was recently open-sourced (it was internally used in Red Hat for scanning RHEL packages for over 10 years. Now, it scans containers, images, and probably more). It can also perform differential scanning. /cc @kdudka

I have also heard questions on whether it works with other hosting platforms or could be used as hooks, others find the differential linting cool and exciting too :)

This is nice, thank you for sharing. :-)

I used to contribute to projects I use when I hit a bug or would like to use a feature. But I find differential linting super useful so I would like to contribute as much as I can.

This is great. I appreciate your contribution and your suggestions they are very valuable. I'm trying to be as responsive as possible, but sometimes it could take me a few days to reply to PRs or Issues since I also have other responsibilities in Red Hat, and this is my sort of personal project.

@jamacku
Copy link
Member

jamacku commented May 3, 2024

I was thinking about refactoring action.yaml a little so it's more compatible with an executable differential-shellcheck and cli args. I came up with the following inputs. What do you think?

inputs:
  base:
    description: Hash of base commit.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}${{ github.event.pull_request.base.sha }}${{ else if github.event_name == 'push' }}${{ github.event.before }}${{ endif }}
  head:
    description: Hash of head commit.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}${{ github.event.pull_request.head.sha }}${{ else if github.event_name == 'push' }}${{ github.event.after }}${{ endif }}

  diff-scan:
    description: Input allowing to request specific type of scan.
    required: false
    default: ${{ if github.event_name == 'pull_request' }}'true'${{ else if github.event_name == 'push' }}'false'${{ endif }}
  strict-check-on-push:
...

I haven't tested your suggestion yet, but it certainly seems possible. I'm just afraid that this might break something since the logic in action.yml is hard to cover by regular tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants