chore(build): Refactor BuildService and separate Maven/Docker specific parts. by rhuss · Pull Request #1116 · fabric8io/docker-maven-plugin

rhuss · 2018-10-18T06:11:51Z

No description provided.

ghost · 2018-10-18T06:13:21Z

-            throw new MojoExecutionException("Cannot create archive " + archive, e);
+            throw new IOException("Cannot create archive " + archive, e);
        } catch (RuntimeException e) {
            e.printStackTrace();


Use a logger to log this exception.

codecov · 2018-10-18T06:13:36Z

Codecov Report

Merging #1116 into master will increase coverage by 0.16%.
The diff coverage is 55.75%.

@@             Coverage Diff              @@
##             master    #1116      +/-   ##
============================================
+ Coverage     52.31%   52.48%   +0.16%     
+ Complexity     1473     1469       -4     
============================================
  Files           150      161      +11     
  Lines          7854     7934      +80     
  Branches       1168     1150      -18     
============================================
+ Hits           4109     4164      +55     
- Misses         3343     3348       +5     
- Partials        402      422      +20

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/io/fabric8/maven/docker/RemoveMojo.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../fabric8/maven/docker/util/ImageNameFormatter.java	`94.11% <ø> (ø)`	`11 <0> (ø)`	⬇️
...r/build/maven/assembly/AllFilesExecCustomizer.java	`0% <ø> (ø)`	`0 <0> (?)`
.../docker/config/handler/property/ValueProvider.java	`86.73% <ø> (ø)`	`11 <0> (ø)`	⬇️
...fabric8/maven/docker/config/run/RestartPolicy.java	`100% <ø> (ø)`	`4 <0> (?)`
...java/io/fabric8/maven/docker/VolumeRemoveMojo.java	`30% <ø> (ø)`	`2 <0> (ø)`	⬇️
...abric8/maven/docker/config/build/AssemblyMode.java	`0% <ø> (ø)`	`0 <0> (?)`
...o/fabric8/maven/docker/util/VolumeBindingUtil.java	`80.39% <ø> (ø)`	`22 <0> (ø)`	⬇️
...ava/io/fabric8/maven/docker/access/UrlBuilder.java	`66.38% <ø> (ø)`	`21 <0> (ø)`	⬇️
...ic8/maven/docker/config/run/WaitConfiguration.java	`72.52% <ø> (ø)`	`10 <0> (?)`
... and 109 more

ghost · 2018-10-18T07:02:32Z


-    public synchronized void watch(WatchContext context, BuildService.BuildContext buildContext, List<ImageConfiguration> images) throws DockerAccessException,
-            MojoExecutionException {
+    public synchronized void watch(WatchContext watchContext, MavenBuildContext buildContext, List<ImageConfiguration> images) throws


Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed.

ghost · 2018-10-19T16:01:08Z

-    @Parameter
    private int retry;

    public RestartPolicy() {};


Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
Remove this empty statement.

ghost · 2018-10-19T16:01:09Z

+		return soft;
+	}
+
+    Pattern ULIMIT_PATTERN = Pattern.compile("^(?<name>[^=]+)=(?<hard>[^:]*):?(?<soft>[^:]*)$");


Rename this field "ULIMIT_PATTERN" to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

ghost · 2018-10-19T16:01:10Z

-            return new BZip2CompressorOutputStream(out);
-        }
-    };
+    none("tar"),


Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.

ghost · 2018-10-19T16:01:10Z

-        }
-    };
+    none("tar"),
+    gzip("tar.gz"),


Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.

ghost · 2018-10-19T16:01:11Z

-    };
+    none("tar"),
+    gzip("tar.gz"),
+    bzip2("tar.bz");


Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.

ghost · 2018-10-19T16:01:12Z

-    private File dockerFileFile, dockerArchiveFile;
-
-    public BuildImageConfiguration() {}
+    public BuildConfiguration() {}


Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.

ghost · 2018-10-19T16:01:13Z

    }

-    public String initAndValidate(Logger log) throws IllegalArgumentException {
+    public String validate() throws IllegalArgumentException {


Remove the declaration of thrown exception 'java.lang.IllegalArgumentException' which is a runtime exception.

ghost · 2018-10-19T16:01:14Z

+        return build;
+    }
+
+    public static class Builder extends  ImageConfiguration.Builder {


Rename this class.

ghost · 2018-10-19T16:01:15Z

+            return nocache.length() == 0 || Boolean.valueOf(nocache);
+        } else {
+            BuildConfiguration buildConfig = imageConfig.getBuildConfiguration();
+            return buildConfig.getNoCache() != null ? buildConfig.getNoCache() : false;


Remove the literal "false" boolean value.

ghost · 2018-10-19T16:01:16Z

-            public Object handleResponse(HttpResponse response) throws IOException {
-                try (InputStream stream = response.getEntity().getContent();
-                     OutputStream out = compression.wrapOutputStream(new FileOutputStream(filename))) {
+    private ResponseHandler<Object> getImageResponseHandler(final String filename) throws FileNotFoundException {


Remove the declaration of thrown exception 'java.io.FileNotFoundException', as it cannot be thrown from method's body.

ghost · 2018-10-19T16:01:17Z

+        return inline;
+    }
+
+    public static class Builder extends AssemblyConfiguration.Builder {


Rename this class.

ghost · 2018-10-19T16:01:17Z

+        return assembly;
+    }
+
+    public static class Builder extends BuildConfiguration.Builder {


Rename this class.

ghost · 2018-10-19T16:01:19Z

-        AssemblyConfiguration config = new AssemblyConfiguration.Builder().ignorePermissions(false).permissions("ignore").build();
-        assertTrue(config.isIgnorePermissions());;
+        AssemblyConfiguration config = new AssemblyConfiguration.Builder().permissions("ignore").build();
+        assertSame(config.getPermissions(), AssemblyConfiguration.PermissionMode.ignore);;


Swap these 2 arguments so they are in the correct order: expected value, actual value.

ghost · 2018-10-19T16:01:21Z


+    private List<ImageConfiguration> convertToPlainImageConfigurations(List<MavenImageConfiguration> images) {
+        if (images == null) {
+            return null;


Return an empty collection instead of null.

ghost · 2018-10-19T16:01:21Z


-    private RunImageConfiguration extractRunConfiguration(ImageConfiguration fromConfig, ValueProvider valueProvider) {
-        RunImageConfiguration config = fromConfig.getRunConfiguration();
+    private RunConfiguration extractRunConfiguration(ImageConfiguration fromConfig, ValueProvider valueProvider) {


Refactor this method to reduce its Cognitive Complexity from 39 to the 15 allowed.

ghost · 2018-10-19T16:01:23Z

-                .exportBasedir(valueProvider.getBoolean(ASSEMBLY_EXPORT_BASEDIR, config == null ? null : config.getExportTargetDir()))
-                .ignorePermissions(valueProvider.getBoolean(ASSEMBLY_IGNORE_PERMISSIONS, config == null ? null : config.getIgnorePermissions()))
-                .permissions(valueProvider.getString(ASSEMBLY_PERMISSIONS, config == null ? null : config.getPermissionsRaw()))
+                .permissions(valueProvider.getString(ASSEMBLY_PERMISSIONS, config == null ? null : config.getPermissions() != null ? config.getPermissions().name() : null))


Extract this nested ternary operation into an independent statement.

ghost · 2018-10-19T16:01:23Z

+                .permissions(valueProvider.getString(ASSEMBLY_PERMISSIONS, config == null ? null : config.getPermissions() != null ? config.getPermissions().name() : null))
                .user(valueProvider.getString(ASSEMBLY_USER, config == null ? null : config.getUser()))
-                .mode(valueProvider.getString(ASSEMBLY_MODE, config == null ? null : config.getModeRaw()))
+                .mode(valueProvider.getString(ASSEMBLY_MODE, config == null ? null : config.getMode() != null ? config.getMode().name() : null))


Extract this nested ternary operation into an independent statement.

ghost · 2018-10-19T16:01:24Z

        // Init and validate configs. After this step, getResolvedImages() contains the valid configuration.
        for (ImageConfiguration imageConfiguration : images) {
-            apiVersion = EnvUtil.extractLargerVersion(apiVersion, imageConfiguration.initAndValidate(nameFormatter, log));
+            for (String version : imageConfiguration.validate(nameFormatter))


Use curly braces or indentation to denote the code conditionally executed by this "for".

ghost · 2018-10-19T16:01:25Z

     */
-    public static String initAndValidate(List<ImageConfiguration> images, String apiVersion, NameFormatter nameFormatter,
+    public static String initAndValidate(List<ImageConfiguration> images, String apiVersion, ImageConfiguration.NameFormatter nameFormatter,
                                         Logger log) {


Remove this unused method parameter "log".

ghost · 2018-10-19T16:01:26Z

     */
-    public File createDockerTarArchive(String imageName, final MojoParameters params, final BuildImageConfiguration buildConfig, Logger log, ArchiverCustomizer finalCustomizer)
-            throws MojoExecutionException {
+    public File createDockerTarArchive(String imageName, final MavenBuildContext context, final BuildConfiguration buildConfig, ArchiverCustomizer finalCustomizer, Logger log)


Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

ghost · 2018-10-19T16:01:26Z


    // visible for testing
-    void verifyGivenDockerfile(File dockerFile, BuildImageConfiguration buildConfig, FixedStringSearchInterpolator interpolator, Logger log) throws IOException {
+    void verifyGivenDockerfile(File dockerFile, BuildConfiguration buildConfig, Function<String, String> interpolator, Logger log) throws IOException {


Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

ghost · 2018-10-19T16:01:27Z

    @Override
-    protected void executeInternal(ServiceHub hub) throws DockerAccessException, MojoExecutionException {
-        MojoParameters params = createMojoParameters();
+    protected void executeInternal(ServiceHub hub) throws IOException {


Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.

ghost · 2018-10-19T16:01:27Z

    private String postExec;

-    public WatchImageConfiguration() {};
+    public WatchConfiguration() {};


Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
Remove this empty statement.

ghost · 2018-10-19T16:01:28Z

     * @deprecated Use &lt;http&gt;&lturl&gt;&lt;/url&gt;&lt;/http&gt; instead
     */
-    @Parameter
    private String url;


Add the missing @deprecated annotation.
Do not forget to remove this deprecated code someday.

ghost · 2018-10-19T16:01:28Z

+ *
 * Copyright 2014 Roland Huss
 *
 * Licensed under the Apache License, Version 2.0 (the "License");


This block of commented-out lines of code should be removed.

ghost · 2018-10-19T16:01:29Z

+
    // Prepare start like creating custom networks, auto pull images, map aliases and return the list of images
    // to start in the correct order
    private Queue<ImageConfiguration> prepareStart(ServiceHub hub, QueryService queryService, RunService runService, Set<String> imageAliases)


Remove this unused method parameter "queryService".

ghost · 2018-10-21T20:41:21Z

-import io.fabric8.maven.docker.util.MojoParameters;
 import io.fabric8.maven.docker.util.GavLabel;
+import io.fabric8.maven.docker.util.Logger;
 import io.fabric8.maven.docker.util.StartOrderResolver;


Remove this unused import 'io.fabric8.maven.docker.util.StartOrderResolver'.

ghost · 2018-10-21T20:41:22Z

+        if (!runConfig.getNetworkingConfig().isCustomNetwork() && runConfig.getLinks() != null) {
+            runConfig.getLinks()
+                     .stream()
+                     .map(s -> !s.contains(":") ? s : s.substring(0, s.lastIndexOf(":")))


Put single-quotes around ':' to use the faster "lastIndexOf(char)" method.

ghost · 2018-10-22T20:44:50Z

+    @Test
+    public void testEmpty() throws Exception {
+        setupDefaultAuthConfigFactory();
+        executeWithTempHomeDir(homeDir -> assertEquals(factory.createAuthConfig(kind, null, "blubberbla:1611"), RegistryAuth.EMPTY_REGISTRY_AUTH));


Swap these 2 arguments so they are in the correct order: expected value, actual value.

ghost · 2018-10-22T20:44:51Z

+
+        executeWithTempHomeDir(homeDir -> {
+            setupServers();
+            assertEquals(factory.createAuthConfig(kind, "roland", "another.repo.org"), RegistryAuth.EMPTY_REGISTRY_AUTH);


Swap these 2 arguments so they are in the correct order: expected value, actual value.

ghost · 2018-10-22T20:44:51Z

@@ -0,0 +1,95 @@
+package io.fabric8.maven.docker.build.maven;
+
+import java.util.Optional;


Remove this unused import 'java.util.Optional'.

ghost · 2018-10-22T20:44:52Z

+public class RegistryAuthConfig {
+
+    private Map<String, Map<String, String>> handlerConfig = new HashMap<>();
+    private Map<Kind, Map<String, String>> kindConfig = new HashMap<>();


Convert this Map to an EnumMap.

ghost · 2018-10-22T20:44:53Z

            return toAuthConfig(creds);
        } catch (IOException e) {
-            throw new MojoExecutionException("Error getting the credentials for " + registryToLookup + " from the configured credential helper",e);
+            throw new RuntimeException("Error getting the credentials for " + registryToLookup + " from the configured credential helper",e);


Define and throw a dedicated exception instead of using a generic one.

ghost · 2018-10-22T20:44:53Z

            return new VersionCommand().getVersion();
        } catch (IOException e) {
-            throw new MojoExecutionException("Error getting the version of the configured credential helper",e);
+            throw new RuntimeException("Error getting the version of the configured credential helper",e);


Define and throw a dedicated exception instead of using a generic one.

ghost · 2018-10-22T20:44:55Z

+        String useOpenAuthMode = registryAuthConfig.extractFromProperties(props, kind, AUTH_USE_OPENSHIFT_AUTH);
+        // Check for system property
+        if (useOpenAuthMode != null) {
+            boolean useOpenShift = Boolean.valueOf(useOpenAuthMode);


Use "Boolean.parseBoolean" for this string-to-boolean conversion.

ghost · 2018-10-22T20:44:55Z

+
+import java.io.Serializable;
+import java.util.Map;
+import java.util.TreeMap;


Remove this unused import 'java.util.TreeMap'.

ghost · 2018-10-22T20:44:56Z

+ */
+public class RegistryAuth {
+
+    public final static RegistryAuth EMPTY_REGISTRY_AUTH =


Reorder the modifiers to comply with the Java Language Specification.

ghost · 2018-10-22T20:44:56Z

+        new RegistryAuth.Builder().username("").password("").email("").auth("").build();
+
+    public static final String USERNAME = "username";
+    public static final String PASSWORD = "password";


'PASSWORD' detected in this expression, review this potentially hard-coded credential.

ghost · 2018-10-22T20:44:57Z

+    public static final String EMAIL = "email";
+    public static final String AUTH = "authToken";
+
+    private String username, password, email, auth, authEncoded;


Rename field "username" to prevent any misunderstanding/clash with field "USERNAME" defined on line 22.
Rename field "password" to prevent any misunderstanding/clash with field "PASSWORD" defined on line 23.
Rename field "email" to prevent any misunderstanding/clash with field "EMAIL" defined on line 24.
Rename field "auth" to prevent any misunderstanding/clash with field "AUTH" defined on line 25.
Declare "password" on a separate line.
Declare "email" on a separate line.
Declare "auth" on a separate line.
Declare "authEncoded" on a separate line.

ghost · 2018-10-22T20:44:57Z

+
+    // ======================================================================================================
+
+    private String createAuthEncoded() {


Move this method into "Builder".

ghost · 2018-10-22T20:44:58Z

+    private RegistryAuth extractAuthConfigFromDocker(JsonObject dockerConfig, String registry) {
+        String registryToLookup = registry != null ? registry : DOCKER_LOGIN_DEFAULT_REGISTRY;
+
+        if (dockerConfig.has("credHelpers") || dockerConfig.has("credsStore")) {


Define a constant instead of duplicating this literal "credsStore" 3 times.
Define a constant instead of duplicating this literal "credHelpers" 3 times.

ghost · 2018-10-22T20:45:05Z

SonarQube analysis reported 153 issues

7 blocker
49 critical
32 major
59 minor
6 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

DockerFileBuilder.java#L259: End this switch case with an unconditional break, return or throw statement.
DockerFileBuilder.java#L262: End this switch case with an unconditional break, return or throw statement.
ExternalCommand.java#L101: Make sure that executing this OS command is safe here.
DockerFileBuilder.java#L341: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
MappingTrackArchiver.java#L127: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
AssemblyConfiguration.java#L151: Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
AssemblyConfiguration.java#L156: Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
AssemblyConfiguration.java#L161: Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
AssemblyConfiguration.java#L166: Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.
AssemblyMode.java#L31: Rename this constant name to match the regular expression '^[A-Z][A-Z0-9](_[A-Z0-9]+)$'.

…c parts. This is part of a larger refactoring story described in fabric8io#1115.

ghost reviewed Oct 18, 2018

View reviewed changes

rhuss force-pushed the pr/1115/refactor-build-service branch from 42077e8 to c7c06c6 Compare October 18, 2018 06:32

ghost reviewed Oct 18, 2018

View reviewed changes

ghost reviewed Oct 19, 2018

View reviewed changes

ghost reviewed Oct 21, 2018

View reviewed changes

ghost reviewed Oct 22, 2018

View reviewed changes

rhuss force-pushed the pr/1115/refactor-build-service branch from 1898156 to 9969472 Compare December 12, 2018 09:45

rhuss added 8 commits December 13, 2018 14:59

chore(build): Refactor BuildService and separate Maven/Docker specifi…

19d1650

…c parts. This is part of a larger refactoring story described in fabric8io#1115.

chore: Sonarqube fixes + 1 bug fixed with buildArgs

308e7e8

chore: Rearranged the config domain model.

c488f43

chore: Continued refactoring

9af4f82

chore(build): Refactored auth handling

a43da56

chore(authentication): Refactored and separated registry authentication

8ecda8e

chore: worked on refactoring build service

bd14f7c

chore: Some minor compilation fixes

90e9dbd

rhuss force-pushed the pr/1115/refactor-build-service branch from 9969472 to 90e9dbd Compare December 13, 2018 14:00

rhuss added WIP Work in Progress size/XXL labels Apr 6, 2019

rhuss self-assigned this Apr 6, 2019

rhuss mentioned this pull request Jun 4, 2020

Podman not supported #1330

Closed

		@@ -0,0 +1,95 @@
		package io.fabric8.maven.docker.build.maven;

		import java.util.Optional;


		// ======================================================================================================

		private String createAuthEncoded() {

Uh oh!

Conversation

rhuss commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 18, 2018 •

edited

Loading