Add implementation for extraction of env and logs from Docker containers

[sigalsax]

This PR adds functionality for extracting env variables and logs from docker containers using the docker-api client.

[Tovli]

Please add unit tests project
Refactor the code to be SOLID (mainly the S)

Understand what happen when you do SpringApplication.run(

[Tovli]

Please read from configurations (note that you usually want to work differently locally and in production)

[Tovli]

This class should contain only the logic that we need to run the application. not business logic
You might put here - general spring boot configurations (DI/logging etc.)

[eranha]

Class documentation looks as follows:
/**

• Foo bar
  */
  Java SDK contains a documentation generator that outputs HTML

[eranha]

IMHO any public method should be documented.

[sigalsax]

do you suggest it they be private? and if so why?

[eranha]

"If you like it than you should have put a ring on it"
If it's public it should be documented.

[eranha]

MIssing spaces.
IS there a reason for the initial value to be an empty string?

[eranha]

Starting an interface name with a capital I is a C# convention.
an interface should have only methods.

[eranha]

Starting an interface name with a capital I is a C# convention.
an interface should have only methods.

[eranha]

As a rule of thumb if you don't plan for this class to be extended, declare it final, if they get injected create a setter for them.

[sigalsax]

is this a rule of thumb for all classes?

[eranha]

Take into consideration that if it's not final it can be extended.

[sigalsax]

why is extended a class an issue?

[eranha]

if write a code that depends on that class, someone may provide a behaviour that do not expect. If you plan to let. others extend the class that is OK.

[eranha]

You are certain that access.getLogs wii not return null and won't trow an exception?

[sigalsax]

If a container exists then there should be at startup logs. In terms of what to return, would I rethrow the null pointer

try {
}  catch (NullPointerException e) {
  throw e
}

or an empty object?

try {
}  catch (NullPointerException e) {
      return new LogsModel();
}

[eranha]

Better to check for null rather than try and fall.

[sigalsax]

check for null or throw null?

[eranha]

It depends on the logic. In java there are two types of exceptions:

1. RuntimeException (means that something unexpected had happened that the user cannot be recovered)
2. CheckedException (you must try or catch or declare that the method throws)
   Usually one don't throw a RuntimeExcpetion and also not Java API exception but creates a BusinessExecption

class UserCreateException extends Exception {}
pubic void registerUser() throws UserCreateException {
 try {
    registerUser();
  } catch (Throwable t) {
   // Throwable is super class of runtime and checked exceptions
    throw new UserCreateException("Failed to create user", t);
  }
}

[eranha]

it would be better off calling this method:
extractsImportantPartsOfEachLogEntry than a name which is not a verb (Methods should be verbs) and a single line comment above the method. Remember the best comment you can write is the one you didn't write.

[sigalsax]

very good call

[eranha]

Here is where this error ends, is this the desired error handling?

[sigalsax]

Maybe just a simple

     } catch (Exception e) {
            System.out.println("Unable to get environment variables from container...");
        }

?

[eranha]

Add error info goes without saying. But I was referring to error handling strategy in general. this code means the error ends here, if you're aware of it and it's OK, then it's OK.

[eranha]

remove the comment and name the method:extractsImportantPartsOfEachEnvironmentEntry

[sigalsax]

What about formatAndPopulateEnvCollection since that is what it is effectively doing? Or does that go against the S in SOLID?

What about for the logs method for extracting and populating the log model. I was thinking extractLogAndPopulateCollection instead of extractImportantPartsOfLogEntry but is that also break single responsibility?

[eranha]

if a method has And in its name, it means it's doing more than one thing and thus violating the first rule of SOLID->SRP
parseAndPopulate should be split to:
parse()
populate()

[eranha]

This method is public, should it be public?. And if it should be public, due to the fact that it does not validate the string or checking the array boundaries, it can easily cause a runtime exception.

[sigalsax]

It should definitely be private since no other classes call this. Updating this one now.

How does public make it easy to cause a runtime exception?

[eranha]

if it's public anyone can access it and get an error

[eranha]

Refrain from using abbreviations use Authentication

[eranha]

Starting an interface name with a capital I is a C# convention.
Missing documentation.

[sigalsax]

I want to differentiate between Interfaces and Regular classes that implement them. Would AuthenticationUserDataModelInterface suffice?

[eranha]

No not in java look at all the interfaces in Java they do not have any name identifier.
Map, Collection, List, Stream etc.

[eranha]

suggested code:

	  String data = "HOSTNAME=253d906eab8e\n" +
                    "CONJUR_DATA_KEY=G2+iUjmifBzFlqXjmQT4+hq2WssOumjRkrzbadvILYg=\n" +
                    "PWD=/opt/conjur-server\n" +
                    "HOME=/root\n" +
                    "DATABASE_URL=postgres://postgres@database/postgres";

        Properties properties = new Properties();
        try {
            properties.load(new StringReader(data));
        } catch (IOException e) {
            throw new ArgumentExcption(String.format("The input is not the expected format, %s", data));
        }

        List<String> includedEnv = Arrays.asList(new String[] {
                "HOSTNAME",
                "DATABASE_URL",
                "PORT",
                "CONJUR_ADMIN_PASSWORD",
                "CONJUR_LOG_LEVEL",
                "CONJUR_PASSWORD",
                "CONJUR_ACCOUNT"
        });

        List<EnvironmentModel> environmentModels = properties.entrySet()
                .stream().filter(e -> includedEnv.contains(e.getKey()))
                .map(e -> new EnvironmentModel(e.getKey().toString(), e.getValue().toString()))
                .collect(Collectors.toList());

        EnvironmentsModel environmentsModel = new EnvironmentsModel();
        environmentModels.addAll(environmentModels);

[sigalsax]

Is the EnvironmentsModel environmentsModel = new EnvironmentsModel(); necessary here?

[sigalsax]

did you mean environmentsModel?

[eranha]

just as a code reference

[sigalsax]

Not sure what you mean here :o

where? Instead of in the block below?

public void onNext(Frame item) {}

[sigalsax]

add new line

[eranha]

This method is public, if the argument is null, you will have a null entry [null] in your list.

[sigalsax]

added a check

    public void add(ConjurLogModel log) {
        if (log != null) {
            logList.add(log);
        }
    }

[sigalsax]

I need the method public to be reachable from the calling class, no?

[eranha]

Replace lambda with method reference

[eranha]

Consider having the script in a separate file.

[eranha]

Namespace is java reads the company domain name in reverse, so it should be com.cyberak.troubleshooing.controller and if it relates only to docker maybe com.cyberak.docker.troubleshooing.controller?