Exit sputnik with error code if checks fail #205

marquiswang · 2019-10-15T17:44:40Z

This allows scripts/tools to take the return result into account.

Furthermore, Extensions of sputnik like the sputnik-maven-plugin can use
a return value of the Engine to communicate the result of a run and do
something like fail a build.

Implement this by adding a Score object (with the review label and the
score value as fields) and return that in Engine.run().

If a failing score value (< 0) is returned, then call System.exit with
the score as the error status.

codecov-io · 2019-10-15T17:48:15Z

Codecov Report

Merging #205 into master will decrease coverage by 0.79%.
The diff coverage is 66.96%.

@@             Coverage Diff             @@
##             master     #205     +/-   ##
===========================================
- Coverage     72.52%   71.72%   -0.8%     
- Complexity      592      608     +16     
===========================================
  Files           142      147      +5     
  Lines          1889     1956     +67     
  Branches        121      129      +8     
===========================================
+ Hits           1370     1403     +33     
- Misses          462      490     +28     
- Partials         57       63      +6

Impacted Files	Coverage Δ	Complexity Δ
...in/java/pl/touk/sputnik/engine/VisitorBuilder.java	`100% <ø> (ø)`	`9 <0> (-5)`	⬇️
...c/main/java/pl/touk/sputnik/review/ReviewFile.java	`92.3% <ø> (ø)`	`7 <0> (ø)`	⬇️
src/main/java/pl/touk/sputnik/engine/Engine.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
src/main/java/pl/touk/sputnik/Main.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/pl/touk/sputnik/connector/Connectors.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
src/main/java/pl/touk/sputnik/review/Review.java	`65% <100%> (ø)`	`13 <1> (+1)`	⬆️
.../java/pl/touk/sputnik/connector/ConnectorType.java	`66.66% <100%> (+2.38%)`	`3 <0> (ø)`	⬇️
...a/pl/touk/sputnik/configuration/GeneralOption.java	`98.36% <100%> (ø)`	`3 <0> (ø)`	⬇️
...ouk/sputnik/engine/score/ScoreStrategyFactory.java	`100% <100%> (ø)`	`7 <7> (?)`
...uk/sputnik/connector/local/LocalFacadeBuilder.java	`14.28% <14.28%> (ø)`	`1 <1> (?)`
... and 16 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 f4d3393...0355151. Read the comment docs.

sputnikci · 2019-10-31T20:21:04Z

src/main/java/pl/touk/sputnik/connector/ConnectorFacadeFactory.java

@@ -15,6 +16,7 @@
    StashFacadeBuilder stashFacadeBuilder = new StashFacadeBuilder();
    GithubFacadeBuilder githubFacadeBuilder = new GithubFacadeBuilder();
    SaasFacadeBuilder saasFacadeBuilder = new SaasFacadeBuilder();
+    LocalFacadeBuilder localFacadeBuilder = new LocalFacadeBuilder();


[Checkstyle] ERROR: Variable 'localFacadeBuilder' must be private and have accessor methods.

sputnikci · 2019-10-31T20:21:05Z

src/main/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilder.java

@@ -11,6 +11,8 @@
 import pl.touk.sputnik.review.Review;
 import pl.touk.sputnik.review.ReviewFile;

+import java.util.ArrayList;


[Checkstyle] ERROR: Unused import - java.util.ArrayList.

SpOOnman

I like your idea. What do you think of a Score object? Can it be better than enum?

SpOOnman · 2019-11-18T20:41:29Z

src/main/java/pl/touk/sputnik/connector/local/LocalFacadeBuilder.java

+
+import java.io.IOException;
+
+public class LocalFacadeBuilder {


Please rebase with sputnik:master so these diffs will go away.

SpOOnman · 2019-11-18T20:45:04Z

src/main/java/pl/touk/sputnik/engine/Engine.java

+        String scoreStrategyName = scoreStrategyName();
+        if (!ScoreStrategy.isValidScoreStrategy(scoreStrategyName)) {
+            log.warn("Score strategy {} not found, using default ScoreAlwaysPass", scoreStrategyName);
+        }
+
+        Score score = ScoreStrategy.of(scoreStrategyName).score(review);
+        review.setScore(score);
+        log.info("{} violations found, {} errors. Adding score {}",
+                review.getTotalViolationCount(),
+                review.getViolationCount().get(Severity.ERROR),
+                score);
+


This looks like a class or a visitor to me. What do you think?

SpOOnman · 2019-11-18T20:46:20Z

src/main/java/pl/touk/sputnik/engine/Engine.java

+    }
+
+    @NotNull
+    private String scoreStrategyName() {


I think this code does not belong to Engine. It should be moved elsewhere.

SpOOnman · 2019-11-18T20:48:51Z

src/main/java/pl/touk/sputnik/engine/score/Score.java

@@ -0,0 +1,9 @@
+package pl.touk.sputnik.engine.score;
+
+public enum Score {


I think we miss opportunity here. There is a degradation of how much information we have. I'm not sure how much information this object should contain for now, but I feel this is too little.

My thought is that old code returned a singleton map of label and score. However, the label and numerical score were never affected by the review processors - it was just configuration that was passed through. The only actual information that is necessary is the pass condition.

In fact, the label and numerical score is unused by all of the ReviewPublishers except for the Gerrit one. This felt more generic.

You are right about previous implementation. I just thought we can pass an object here with enum inside and some other information in future.

SpOOnman · 2019-11-18T20:55:36Z

src/main/java/pl/touk/sputnik/engine/score/ScoreStrategies.java

+    }
+
+    @NotNull
+    private ScoreStrategy getScoreStrategy(String aScoreStrategy, String aS) {


aScoreStrategy is not used here and aS is too short for a parameter name.

How embarrassing. I'll fix this.

Ok, thank you.

SpOOnman · 2019-11-18T20:56:30Z

src/main/java/pl/touk/sputnik/engine/score/ScoreStrategy.java

+    };
+
+
+    private static final String NOSCORE = "NOSCORE";


There is a code duplication here. It's the same code as in ScoreStrategies.

SpOOnman · 2019-11-18T20:57:03Z

src/main/java/pl/touk/sputnik/engine/score/ScoreStrategy.java

+
+import java.util.Set;
+
+public enum ScoreStrategy {


I am not happy with this refactoring. This is a business login implementation within enum methods. You've moved builder into a map. I prefer it the way it was before.

The intention was to use implement a strategy pattern with enums, since there's no state so there only needs to be one instance of each one. I can revert this to ScoreStrategy being an interface with 4 implementations if you prefer. You no longer need a builder because there's nothing to build.

The advantage of this is that you can still do logging. Yeah that's a good advantage. I will do that.

SpOOnman · 2019-11-18T21:00:17Z

src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java

+    }
+
+    @NotNull
+    private GerritScoreLabeler scoreLabeler(Configuration configuration) {


I think this fits more to ReviewInputBuilder.

SpOOnman · 2019-11-25T07:30:39Z

I've made a change in master branch that fixed one but so I guess you must rebase again.

Robot comments show up in the Findings tab, instead of in the comments. This makes it much easier to differentiate from them a reviewer's comments. The API requires a "runId" for the comment. I've chosen to just send a random UUID for now, but eventually we could wire in something like a build number or something.

marquiswang force-pushed the master branch from f72a6a2 to 8a27c63 Compare October 15, 2019 19:19

marquiswang mentioned this pull request Oct 25, 2019

Add "local" connector type #206

Merged

marquiswang force-pushed the master branch from 8a27c63 to 3e0dc9d Compare October 31, 2019 20:17

sputnikci reviewed Oct 31, 2019

View reviewed changes

marquiswang force-pushed the master branch from 3e0dc9d to 0355151 Compare November 11, 2019 21:30

SpOOnman requested changes Nov 18, 2019

View reviewed changes

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

marquiswang force-pushed the master branch 2 times, most recently from b3bf26d to 0f22161 Compare April 23, 2021 17:35

marquiswang force-pushed the master branch from 0f22161 to ec75313 Compare April 23, 2021 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exit sputnik with error code if checks fail #205

Exit sputnik with error code if checks fail #205

marquiswang commented Oct 15, 2019

codecov-io commented Oct 15, 2019 •

edited

sputnikci Oct 31, 2019

sputnikci Oct 31, 2019

SpOOnman left a comment

SpOOnman Nov 18, 2019

SpOOnman Nov 18, 2019

SpOOnman Nov 18, 2019

SpOOnman Nov 18, 2019

marquiswang Nov 18, 2019

SpOOnman Nov 25, 2019

SpOOnman Nov 18, 2019

marquiswang Nov 18, 2019

SpOOnman Nov 25, 2019

SpOOnman Nov 18, 2019

SpOOnman Nov 18, 2019

marquiswang Nov 18, 2019

SpOOnman Nov 25, 2019

SpOOnman Nov 18, 2019

SpOOnman commented Nov 25, 2019


		import java.io.IOException;

		public class LocalFacadeBuilder {

		@@ -0,0 +1,9 @@
		package pl.touk.sputnik.engine.score;

		public enum Score {

Exit sputnik with error code if checks fail #205

Are you sure you want to change the base?

Exit sputnik with error code if checks fail #205

Conversation

marquiswang commented Oct 15, 2019

codecov-io commented Oct 15, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpOOnman commented Nov 25, 2019

codecov-io commented Oct 15, 2019 •

edited