Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

EOS-27541: Add Github Action for shellcheck #58

Merged
merged 3 commits into from
Feb 1, 2022
Merged

EOS-27541: Add Github Action for shellcheck #58

merged 3 commits into from
Feb 1, 2022

Conversation

keithpine
Copy link
Contributor

Lint all bash scripts using shellcheck for any push to main or integration branches and PRs.

Some extra strict options are enabled in .shellcheckrc (v0.8.0 required). All of the enabled checks cover some aspects of several Bash style guides, although they aren't all identical:

For now, the Action is configured to always succeed because of existing failures.

Signed-off-by: Keith Pine [email protected]

Lint all bash scripts using [shellcheck](https://github.com/koalaman/shellcheck)
for any push to main or integration branches and PRs.

Some extra strict options are enabled in .shellcheckrc. All of the
enabled checks include some elements of several Bash style guides,
although they don't follow all exactly:

  - [bahamas10/bash-style-guide](https://github.com/bahamas10/bash-style-guide)
  - [BashGuide/Practices](http://mywiki.wooledge.org/BashGuide/Practices)
  - [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html)

For now the Action is configured to always succeed because of existing
failures.

Signed-off-by: Keith Pine <[email protected]>
@keithpine keithpine changed the title Add Github Action for shellcheck EOS-27541: Add Github Action for shellcheck Jan 22, 2022
@walterlopatka
Copy link
Contributor

LGTM (though I admit I don't have experience with this)

@johnbent
Copy link
Contributor

@keithpine and @walterlopatka , I think this might be redundant since Codacy is already attached to this repo.

Here is a picture showing the Codacy badge attached to the front README of this repo.
image
If you click on the badge, it will bring you to the Codacy dashboard for this repo.

You'll also notice that Codacy is already doing checks on each pull request:
image

And, finally, this page suggests to me that Codacy includes shellcheck:
https://docs.codacy.com/related-tools/codacy-plugin-tools/

@johnbent
Copy link
Contributor

Confirmed. I logged in to Codacy and looked at settings for the k8s repo and here is what I see:

image

@keithpine
Copy link
Contributor Author

@johnbent Thanks for that info. A couple of questions:

  1. The version of Shellcheck being used is quite old, can it be upgraded? There are a couple of style checks that I've enabled that are not supported in the Codacy version.
  2. Looks like the Codacy run is not flagging the same number of issues, the issue counts are significantly less than what the manual run sees. Can we enable all of the checks (and reduce if needed)? Do you know how the config file setting works? Will it select the .shellcheckrc file if it's in the repository, that way it could be managed w/o access to the Codacy admin panel.
  3. How do we get Codacy to fail the PR check if it detects these issues in the future?

@johnbent
Copy link
Contributor

Plus @mukul-seagate11 . Mukul, can you help answer Keith's questions above about Codacy?

@mukul-seagate11 mukul-seagate11 self-assigned this Jan 25, 2022
Copy link
Contributor

@osowski osowski left a comment

Choose a reason for hiding this comment

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

I'm good with this as a practice. I understand Codacy may run some of this, but I would prefer to control it with a finer-grained approach and then remove it when we have minimal (if any) shell scripts in the future.

The only change I would request is that we need to fork https://github.com/ludeeus/action-shellcheck in a github.com/Seagate repository and reference it in the uses path that way, instead of directly referencing the remote repository to run code in our pipeline unsupervised.

@mukul-seagate11
Copy link
Contributor

I'm good with this as a practice. I understand Codacy may run some of this, but I would prefer to control it with a finer-grained approach and then remove it when we have minimal (if any) shell scripts in the future.

The only change I would request is that we need to fork https://github.com/ludeeus/action-shellcheck in a github.com/Seagate repository and reference it in the uses path that way, instead of directly referencing the remote repository to run code in our pipeline unsupervised.

Sure, let me address the same with. https://github.com/Seagate/action-shellcheck

@mukul-seagate11
Copy link
Contributor

@johnbent Thanks for that info. A couple of questions:

  1. The version of Shellcheck being used is quite old, can it be upgraded? There are a couple of style checks that I've enabled that are not supported in the Codacy version.
  2. Looks like the Codacy run is not flagging the same number of issues, the issue counts are significantly less than what the manual run sees. Can we enable all of the checks (and reduce if needed)? Do you know how the config file setting works? Will it select the .shellcheckrc file if it's in the repository, that way it could be managed w/o access to the Codacy admin panel.
  3. How do we get Codacy to fail the PR check if it detects these issues in the future?

Unfortunately, at the moment Codacy does not support configuration files for Shellcheck but yes can be addressed via github actions for Shellcheck

@mukul-seagate11
Copy link
Contributor

Shellcheck workflow has been addressed via #71

- Use Seagate fork of action
- Add license and copyright
- Remove workflow_dispatch

Signed-off-by: Keith Pine <[email protected]>
@cla-bot cla-bot bot added the cla-signed label Feb 1, 2022
Signed-off-by: Keith Pine <[email protected]>
Copy link
Contributor

@osowski osowski left a comment

Choose a reason for hiding this comment

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

lgtm

@osowski osowski merged commit c1cde23 into Seagate:integration Feb 1, 2022
@keithpine keithpine deleted the EOS-27541_shellcheck branch February 1, 2022 19:01
walterlopatka added a commit that referenced this pull request Feb 2, 2022
* EOS-27575: Avoid capturing container logs as variables (#60)

Sometimes the log output from a container is so large that capturing it
in a variable and printing it will cause bash to crash. Instead of
capturing, just redirect the log output directly to the destination
file. This does have a side-effect of creating an empty log file if a
pod/container has no logs, where previously it would skip the file if
the logs were empty.

Other fixes:

 - Remove leading './' from paths in the output tar file
 - Fix some Shellcheck complaints
 - Use long options for tar and cortx_support_bundle commands
   (script self-documentation)
 - Reduce some code duplication

Signed-off-by: Keith Pine <[email protected]>

* Updated GitHub Actions to include integration branch

Signed-off-by: Rick Osowski <[email protected]>

* UDX-6683 move documentation to root README (#64)

* Initial README updates

Signed-off-by: Rick Osowski <[email protected]>

* Second pass at documentation

Signed-off-by: Rick Osowski <[email protected]>

* Moved README to repo root / merged CODACY

Signed-off-by: Rick Osowski <[email protected]>

* Final pass at README.md

Signed-off-by: Rick Osowski <[email protected]>

* Update Codacy Markdown warnings

Signed-off-by: Rick Osowski <[email protected]>

* Initial review updates from squad

Signed-off-by: Rick Osowski <[email protected]>

* Update Codacy Markdown warnings, Take Two

Signed-off-by: Rick Osowski <[email protected]>

* EOS-28404: Remove unneeded services: cortx-control-headless / clusterip

Signed-off-by: Walter Lopatka <[email protected]>

* EOS-27541: Add Github Action for shellcheck (#58)

* Add Github Action for shellcheck

Lint all bash scripts using [shellcheck](https://github.com/koalaman/shellcheck)
for any push to main or integration branches and PRs.

Some extra strict options are enabled in .shellcheckrc. All of the
enabled checks include some elements of several Bash style guides,
although they don't follow all exactly:

  - [bahamas10/bash-style-guide](https://github.com/bahamas10/bash-style-guide)
  - [BashGuide/Practices](http://mywiki.wooledge.org/BashGuide/Practices)
  - [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html)

For now the Action is configured to always succeed because of existing
failures.

Signed-off-by: Keith Pine <[email protected]>

* Updates

- Use Seagate fork of action
- Add license and copyright
- Remove workflow_dispatch

Signed-off-by: Keith Pine <[email protected]>

* Fix copyright year

Signed-off-by: Keith Pine <[email protected]>

* v0.0.21 (#72)

* Prep v0.0.21

Signed-off-by: Walter Lopatka <[email protected]>

* Prep for v0.0.21

Fixed container version name

Signed-off-by: Walter Lopatka <[email protected]>

* Patch for README.md GitHub formatting (#74)

Signed-off-by: Rick Osowski <[email protected]>

* Update .gitignore file (#76)

Add auto-generated deployment files

Signed-off-by: Keith Pine <[email protected]>

* Update cortx-aws-k8s-installation.md (#78)

Improved formatting.

Co-authored-by: John Bent <[email protected]>

Co-authored-by: Keith Pine <[email protected]>
Co-authored-by: Rick Osowski <[email protected]>
Co-authored-by: John Bent <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants