Add shellcheck support #190

dswiecki · 2018-06-29T20:41:17Z

This PR adds https://github.com/koalaman/shellcheck integration

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckResultParserTest.java

codecov-io · 2018-06-29T20:46:29Z

Codecov Report

Merging #190 (5fa1143) into master (aa51743) will increase coverage by 2.65%.
The diff coverage is 77.63%.

@@             Coverage Diff              @@
##             master     #190      +/-   ##
============================================
+ Coverage     72.38%   75.03%   +2.65%     
- Complexity      608      659      +51     
============================================
  Files           146      154       +8     
  Lines          1988     2079      +91     
  Branches        131      134       +3     
============================================
+ Hits           1439     1560     +121     
+ Misses          489      456      -33     
- Partials         60       63       +3

Impacted Files	Coverage Δ	Complexity Δ
...ocessor/shellcheck/ShellcheckProcessorFactory.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...tnik/processor/shellcheck/ShellcheckException.java	`50.00% <50.00%> (ø)`	`1.00 <1.00> (?)`
...k/processor/shellcheck/json/ShellcheckMessage.java	`54.54% <54.54%> (ø)`	`6.00 <6.00> (?)`
...tnik/processor/shellcheck/ShellcheckProcessor.java	`76.92% <76.92%> (ø)`	`6.00 <6.00> (?)`
...utnik/processor/shellcheck/ShellcheckExecutor.java	`85.71% <85.71%> (ø)`	`5.00 <5.00> (?)`
...k/processor/shellcheck/ShellcheckResultParser.java	`91.66% <91.66%> (ø)`	`10.00 <10.00> (?)`
...a/pl/touk/sputnik/configuration/GeneralOption.java	`98.50% <100.00%> (+0.09%)`	`3.00 <0.00> (ø)`
...ain/java/pl/touk/sputnik/exec/ExternalProcess.java	`76.92% <100.00%> (+76.92%)`	`4.00 <0.00> (+4.00)`
...ava/pl/touk/sputnik/review/filter/ShellFilter.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa51743...5fa1143. Read the comment docs.

SpOOnman · 2021-01-21T22:44:22Z

Hej @dswiecki do you want to continue work on this PR?

dswiecki · 2021-01-27T08:04:15Z

@SpOOnman I've updated PR

SpOOnman

Thank you for your support!

I think there is a problem with shellcheck as it is a shell script. It is hard to run tests with it because we need an environment that has it installed. Please correct me if I'm wrong as I don't know this tool.

SpOOnman · 2021-01-28T22:21:04Z

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckExecutor.java

+    }
+
+    String runOnFile(String filePath) {
+        log.info("Review on file: " + filePath);


Please use {} for string interpolation here

SpOOnman · 2021-01-28T22:22:12Z

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckResultParser.java

+            return Collections.emptyList();
+        }
+        try {
+            List<Violation> violations = new ArrayList<>();


Would it be more readable with Java Stream API?

SpOOnman · 2021-01-28T22:22:58Z

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessorTest.java

+    }
+
+    @Test
+    public void shouldReturnNoViolationsWhenThereIsNoFileToReview() {


Why there is no Assumption here?

In that case, the file to review does not exist thus the processor is not invoked

SpOOnman · 2021-01-28T22:24:15Z

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessorTest.java

+
+    @Test
+    public void shouldReturnOneViolationsForFile() {
+        Assumptions.assumeTrue(isShellcheckInstalled());


This test will pass only if shellcheck is intalled, right?

yes, otherwise it is ignored

SpOOnman · 2021-01-28T22:24:47Z

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckProcessorTest.java

+    private boolean isShellcheckInstalled() {
+        try {
+            String result = new ExternalProcess().executeCommand("shellcheck");
+            return "".equals(result);


return StringUtils.isBlank(result) maybe?

SpOOnman · 2021-01-28T22:26:12Z

src/main/java/pl/touk/sputnik/processor/shellcheck/ShellcheckExecutor.java

+
+    String runOnFile(String filePath) {
+        log.info("Review on file: " + filePath);
+        return new ExternalProcess().executeCommand(buildParams(filePath));


What if shellcheck is not installed?

Then ExternalPorcess throws ExternalProcessException which is not handled specially like in the case of other ProcessorRunningExternalProcess processors

dswiecki · 2021-02-12T15:24:57Z

@SpOOnman on travis-ci there's shellcheck script installed by default

SpOOnman · 2021-03-07T10:42:57Z

Thank you!

austinarrowsmith · 2021-12-20T03:58:38Z

@dswiecki - how do you now run sputnik with a shellcheck-enabled system? I've tried setting shellcheck.enabled=true in the sputnik.conf file, but it doesn't appear to be executed. Can you help advise what's needed? I'm happy to push in another README documentation patch.

* Add basic shellcheck support * Add shellcheck exclude rules * Update junit annotations * Shellcheck review

sputnikci reviewed Jun 29, 2018

View reviewed changes

src/test/java/pl/touk/sputnik/processor/shellcheck/ShellcheckResultParserTest.java Outdated Show resolved Hide resolved

TouK deleted a comment from pjagielski Jul 4, 2018

SpOOnman force-pushed the master branch from 6694cfb to 4d9cc62 Compare January 13, 2021 14:11

dswiecki added 3 commits January 26, 2021 21:46

Add basic shellcheck support

8185397

Add shellcheck exclude rules

90c77ac

Update junit annotations

1e481b0

dswiecki force-pushed the master branch from 635ce03 to 1e481b0 Compare January 26, 2021 21:32

SpOOnman requested changes Jan 28, 2021

View reviewed changes

Shellcheck review

5fa1143

dswiecki requested a review from SpOOnman February 12, 2021 15:47

SpOOnman merged commit a14d3a2 into TouK:master Mar 7, 2021

rufuslevi pushed a commit to rufuslevi/sputnik that referenced this pull request Mar 12, 2024

Add shellcheck support (TouK#190)

345fcc8

* Add basic shellcheck support * Add shellcheck exclude rules * Update junit annotations * Shellcheck review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shellcheck support #190

Add shellcheck support #190

dswiecki commented Jun 29, 2018

codecov-io commented Jun 29, 2018 •

edited

Loading

SpOOnman commented Jan 21, 2021

dswiecki commented Jan 27, 2021

SpOOnman left a comment

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

SpOOnman Jan 28, 2021

dswiecki Feb 12, 2021

dswiecki commented Feb 12, 2021

SpOOnman commented Mar 7, 2021

austinarrowsmith commented Dec 20, 2021

Add shellcheck support #190

Add shellcheck support #190

Conversation

dswiecki commented Jun 29, 2018

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

SpOOnman commented Jan 21, 2021

dswiecki commented Jan 27, 2021

SpOOnman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dswiecki commented Feb 12, 2021

SpOOnman commented Mar 7, 2021

austinarrowsmith commented Dec 20, 2021

codecov-io commented Jun 29, 2018 •

edited

Loading