Skip to content

Add BASH syntax check to linter#3369

Merged
casperklein merged 4 commits intodocker-mailserver:masterfrom
casperklein:lint-basic-bash-check
May 27, 2023
Merged

Add BASH syntax check to linter#3369
casperklein merged 4 commits intodocker-mailserver:masterfrom
casperklein:lint-basic-bash-check

Conversation

@casperklein
Copy link
Copy Markdown
Member

@casperklein casperklein commented May 26, 2023 •
edited
Loading

Description

This PR adds an additional (syntax) check for our BASH scripts.

In my experience, shellcheck does catch syntax errors, however the error message may be not as clear compared to a dedicated syntax check with bash -n.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@casperklein casperklein self-assigned this May 26, 2023
@casperklein casperklein added this to the v13.0.0 milestone May 26, 2023
@casperklein casperklein added area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels May 26, 2023
@casperklein casperklein marked this pull request as ready for review May 26, 2023 12:40
@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented May 26, 2023

In my experience, shellcheck does catch syntax errors, however the error message may be not as clear compared to a dedicated syntax check with bash -n.

I got to be honest: I don't actually see any value in adding this check at the moment. To me

 In ./target/scripts/startup/setup.d/mail_state.sh line 37:
    for SERVICEFILE in "${SERVICEFILES[@]}";; do
    ^-- SC1073 (error): Couldn't parse this for loop. Fix to allow more checks.
                                           ^-- SC1058 (error): Expected 'do'.

is absolutely fine, and

lo.sh: line 3: syntax error near unexpected token `;;'
lo.sh: line 3: `for SERVICEFILE in "${SERVICEFILES[@]}";; do'

really adds no new information IMO.

@casperklein
Copy link
Copy Markdown
Member Author

casperklein commented May 27, 2023

Another point is, the syntax check is pretty fast, less than a second on my setup. It runs before shellcheck and let the lint test fail much faster.
Shellcheck on the other hand takes about 45sec to run.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented May 27, 2023 •
edited
Loading

Fair enough! And I mean, if it really helps others, I don't want to block it anyway :)

georglauterbach
georglauterbach approved these changes May 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

#3370 is the last PR (together with this one) blocking v13.0.0 - please give it a quick review when you've got time @casperklein :)

@casperklein casperklein merged commit 3d6260a into docker-mailserver:master May 27, 2023
@casperklein casperklein deleted the lint-basic-bash-check branch May 27, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@georglauterbach georglauterbach georglauterbach approved these changes

@polarathene polarathene Awaiting requested review from polarathene

Assignees

@casperklein casperklein

Labels

area/tests kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Milestone

v13.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@casperklein @georglauterbach