feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 #1193

sunix · 2022-01-05T15:09:08Z

Description

Log informative message when maven jkube goals are skipped.

part of #1142

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

I have read the contributing guidelines
I signed-off my commit with a user that has signed the Eclipse Contributor Agreement
I Added CHANGELOG entry
I have implemented unit tests to cover my changes
I have updated the documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

manusa · 2022-01-05T15:09:35Z

Eclipse JKube CI Report

Started new GH workflow run for #1193 (2022-01-21T15:24:16Z)

⚙️ JKube E2E Tests (1729317090)

rohanKanojia · 2022-01-05T17:00:00Z

a test seems to be failing:

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.83 s - in org.eclipse.jkube.maven.plugin.mojo.develop.DebugMojoTest
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   HelmPushMojoTest.execute_withSkip_shouldSkipExecution:197 » NullPointer
[INFO] 
[ERROR] Tests run: 36, Failures: 0, Errors: 1, Skipped: 0
[INFO] 
[INFO] ---------------------------------------------------------------------

sunix · 2022-01-05T17:04:28Z

There is a failing test because I introduced mojoExecution that is null in one of the existing test. Trying to fix it.

rohanKanojia · 2022-01-05T17:20:24Z

Although the current log message is very good. While testing I had a feeling if we really need to do the extra effort of specifying goal name in skip message since the goal getting executed is specified by maven on a line above in logs.

Right now we're doing this:

$ mvn k8s:build k8s:resource k8s:apply -Djkube.version=1.6.0-SNAPSHOT -Djkube.skip=true -Pkubernetes
[INFO] Scanning for projects...
[INFO] 
[INFO] ----------< org.eclipse.jkube.quickstarts.maven:spring-boot >-----------
[INFO] Building Eclipse JKube :: Quickstarts :: Maven :: Spring Boot Web 1.5.1
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:build (default-cli) @ spring-boot ---
[INFO] k8s: `k8s:build` goal is skipped.
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:resource (default-cli) @ spring-boot ---
[WARNING] k8s: Cannot access cluster for detecting mode: Unknown host kubernetes.default.svc: Name or service not known
[INFO] k8s: `k8s:resource` goal is skipped.
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:apply (default-cli) @ spring-boot ---
[WARNING] k8s: Cannot access cluster for detecting mode: Unknown host kubernetes.default.svc
[INFO] k8s: `k8s:apply` goal is skipped.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.369 s
[INFO] Finished at: 2022-01-05T22:40:46+05:30
[INFO] ------------------------------------------------------------------------

Does it make sense to just keep a simple message like this? If not, we're good with current implementation as well.

$ mvn k8s:build k8s:resource k8s:apply -Djkube.version=1.6.0-SNAPSHOT -Djkube.skip=true -Pkubernetes
[INFO] Scanning for projects...
[INFO] 
[INFO] ----------< org.eclipse.jkube.quickstarts.maven:spring-boot >-----------
[INFO] Building Eclipse JKube :: Quickstarts :: Maven :: Spring Boot Web 1.5.1
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:build (default-cli) @ spring-boot ---
[INFO] k8s: Skipped.
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:resource (default-cli) @ spring-boot ---
[WARNING] k8s: Cannot access cluster for detecting mode: Unknown host kubernetes.default.svc: Name or service not known
[INFO] k8s: Skipped
[INFO] 
[INFO] --- kubernetes-maven-plugin:1.6.0-SNAPSHOT:apply (default-cli) @ spring-boot ---
[WARNING] k8s: Cannot access cluster for detecting mode: Unknown host kubernetes.default.svc
[INFO] k8s: Skipped
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.410 s
[INFO] Finished at: 2022-01-05T22:42:35+05:30
[INFO] ------------------------------------------------------------------------

sunix · 2022-01-05T18:12:35Z

from the user perspective kubernetes-maven-plugin:1.6.0-SNAPSHOT:resource is not as straightforward as k8s:resource. I personally prefer mentioning the goal that is skipped rather than just Skipped.

rohanKanojia · 2022-01-05T18:46:10Z

okay, let's keep it the way it is 👍

codecov · 2022-01-06T13:39:49Z

Codecov Report

Merging #1193 (a58d233) into master (d8825b7) will increase coverage by 0.02%.
The diff coverage is 31.70%.

@@             Coverage Diff              @@
##             master    #1193      +/-   ##
============================================
+ Coverage     49.57%   49.60%   +0.02%     
- Complexity     3656     3663       +7     
============================================
  Files           455      455              
  Lines         20625    20631       +6     
  Branches       2814     2814              
============================================
+ Hits          10225    10234       +9     
+ Misses         9315     9311       -4     
- Partials       1085     1086       +1

Impacted Files	Coverage Δ
...be/maven/plugin/mojo/build/AbstractDockerMojo.java	`17.92% <18.75%> (+3.02%)`	⬆️
...ube/maven/plugin/mojo/build/AbstractJKubeMojo.java	`79.54% <77.77%> (+0.97%)`	⬆️
...lipse/jkube/maven/plugin/mojo/build/BuildMojo.java	`8.33% <0.00%> (ø)`

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 d8825b7...a58d233. Read the comment docs.

sunix · 2022-01-07T15:47:18Z

the codecov/patch error is caused by existing lines I have moved to a dedicated method ....

manusa · 2022-01-20T10:42:47Z

...lugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/AbstractDockerMojo.java

+    @Override
+    public final void execute() throws MojoExecutionException, MojoFailureException {
+        init();
+        if (!canExecute()) {
+            log.info("`%s` goal is skipped.", mojoExecution.getMojoDescriptor().getFullGoalName());
+            return;
+        }
+        doExecute();
+    }


I understand that this reorganization is similar to what was discussed in #1198 (review)

However, there's already an issue to cover this (#147). Code reorganization at this point might make it harder to refactor later on. Especially since this class now has an execute, executeInternal, and doExecute methods with no clear separation of purposes (at least semantically speaking).

For the current PR I would rather not change (complicate) more the structure of the class (although I do agree the execute method is more readable now), end delay that effort until the whole refactor (+ maybe removal) of the class.

semantically:

execute is the public method

executeInternal is the execution implementation extended by subclasses -> should be abstract

I added doExecute which is the execution code from the abstract class. executed if not skipped

Basically at the end of the day after refactoring
execute()
should call doExecute
that should call executeInternal.

I am open to a better naming or suggestions.

I understand what each does, but there's no clear separation of concerns. If all of the methods are executing stuff, why are you separating two of them... i.e. naming is fine, but separation isn't.

Anyway, this refactor right now introduces more burden if we want to tackle #147, which is the cleaner approach. The point here would be to add the simple log message and try to move on without changing too much what we already have. IMO it would be better to spend the time creating tests that would ensure a successful latter refactor.

I initially refactored because it was easier to test.
I refactored again. That should help to tackle #147 IMHO

I think I might not have explained myself clear.

If the refactor is required to improve or ease the test scenarios, then it's OK, period.

If the refactor or reorganization is to clean up, improve readability etc. what I was trying to say is that this effort will be wasted if we finally implement 147 (which we should). It might even complicate things further. Efforts should be pushed towards testing the current functionalities of the final Mojos so that the upcoming refactor can be done in a safe way.

Anyway, it's OK as it is. Let's move on.

sunix · 2022-01-21T12:57:56Z

@manusa should be ok to merge ?

manusa · 2022-01-21T14:17:41Z

@manusa should be ok to merge ?

I'm not sure if this one can be removed:
https://sonarcloud.io/project/issues?id=jkubeio_jkube&pullRequest=1193&resolved=false&types=CODE_SMELL

sunix · 2022-01-21T14:19:59Z

@manusa should be ok to merge ?

I'm not sure if this one can be removed: https://sonarcloud.io/project/issues?id=jkubeio_jkube&pullRequest=1193&resolved=false&types=CODE_SMELL

let me give a try

…e skipped eclipse-jkube#1142 Signed-off-by: Sun Tan <sutan@redhat.com>

…ractDockerMojo for readability Signed-off-by: Sun Tan <sutan@redhat.com>

sonarcloud · 2022-01-21T15:15:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

89.5% Coverage
0.0% Duplication

sunix · 2022-01-21T15:46:40Z

@manusa should be ok to merge ?

I'm not sure if this one can be removed: https://sonarcloud.io/project/issues?id=jkubeio_jkube&pullRequest=1193&resolved=false&types=CODE_SMELL

fixed

sunix force-pushed the maven-log-informative-message-if-skipped-goal branch 2 times, most recently from 0de8027 to b1f318d Compare January 5, 2022 15:14

sunix changed the title ~~fix: log informative message when maven jkube goals are skipped #1142~~ feat: log informative message when maven jkube goals are skipped #1142 Jan 5, 2022

sunix force-pushed the maven-log-informative-message-if-skipped-goal branch from b1f318d to adb2b15 Compare January 5, 2022 15:19

sunix changed the title ~~feat: log informative message when maven jkube goals are skipped #1142~~ feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 Jan 5, 2022

rohanKanojia requested a review from manusa January 5, 2022 15:25

rohanKanojia added the changelog_missing label Jan 5, 2022

rohanKanojia approved these changes Jan 5, 2022

View reviewed changes

sunix force-pushed the maven-log-informative-message-if-skipped-goal branch from adb2b15 to f98a290 Compare January 6, 2022 13:23

sunix mentioned this pull request Jan 10, 2022

chore(changelog): Adding changelog for issue #1142 Log informative message whenever a goal/task is skipped #1199

Merged

12 tasks

manusa reviewed Jan 20, 2022

View reviewed changes

sunix force-pushed the maven-log-informative-message-if-skipped-goal branch 4 times, most recently from 5a4634b to c82d91d Compare January 21, 2022 12:18

sunix added 2 commits January 21, 2022 15:00

feat(maven-plugin): log informative message when maven jkube goals ar…

746df7b

…e skipped eclipse-jkube#1142 Signed-off-by: Sun Tan <sutan@redhat.com>

chore(maven-plugin): reordering methods of AbstractJKubeMojo and Abst…

a58d233

…ractDockerMojo for readability Signed-off-by: Sun Tan <sutan@redhat.com>

sunix force-pushed the maven-log-informative-message-if-skipped-goal branch from c82d91d to a58d233 Compare January 21, 2022 15:03

manusa approved these changes Jan 24, 2022

View reviewed changes

manusa removed the changelog_missing label Jan 24, 2022

manusa added this to the 1.6.0 milestone Jan 24, 2022

manusa merged commit 66fb1a6 into eclipse-jkube:master Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 #1193

feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 #1193

sunix commented Jan 5, 2022 •

edited by manusa

manusa commented Jan 5, 2022 •

edited

rohanKanojia commented Jan 5, 2022

sunix commented Jan 5, 2022

rohanKanojia commented Jan 5, 2022 •

edited

sunix commented Jan 5, 2022 •

edited

rohanKanojia commented Jan 5, 2022

codecov bot commented Jan 6, 2022 •

edited

sunix commented Jan 7, 2022

manusa Jan 20, 2022

sunix Jan 20, 2022

manusa Jan 20, 2022

sunix Jan 20, 2022

manusa Jan 20, 2022

sunix commented Jan 21, 2022

manusa commented Jan 21, 2022

sunix commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

sunix commented Jan 21, 2022

feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 #1193

feat(maven-plugin): log informative message when maven jkube goals are skipped #1142 #1193

Conversation

sunix commented Jan 5, 2022 • edited by manusa

Description

Type of change

Checklist

manusa commented Jan 5, 2022 • edited

Eclipse JKube CI Report

rohanKanojia commented Jan 5, 2022

sunix commented Jan 5, 2022

rohanKanojia commented Jan 5, 2022 • edited

sunix commented Jan 5, 2022 • edited

rohanKanojia commented Jan 5, 2022

codecov bot commented Jan 6, 2022 • edited

Codecov Report

sunix commented Jan 7, 2022

manusa Jan 20, 2022

Choose a reason for hiding this comment

sunix Jan 20, 2022

Choose a reason for hiding this comment

manusa Jan 20, 2022

Choose a reason for hiding this comment

sunix Jan 20, 2022

Choose a reason for hiding this comment

manusa Jan 20, 2022

Choose a reason for hiding this comment

sunix commented Jan 21, 2022

manusa commented Jan 21, 2022

sunix commented Jan 21, 2022

sonarcloud bot commented Jan 21, 2022

sunix commented Jan 21, 2022

sunix commented Jan 5, 2022 •

edited by manusa

manusa commented Jan 5, 2022 •

edited

rohanKanojia commented Jan 5, 2022 •

edited

sunix commented Jan 5, 2022 •

edited

codecov bot commented Jan 6, 2022 •

edited