Fix for using the official mysql image in composite stacks

[kaloyan-raev]

What does this PR do?

As discussed in #2991 Che has a problem running the official mysql docker image and works the issue around by crafting a custom codenvy/mysql.

I investigated the issue and found that Che is passing empty arrays for the command and the entrypoint parameters of the /containers/create Docker API method. This overrides the default entrypoint and command of the mysql docker image, which leads to failure during the container startup. Instead of passing empty arrays for these parameters, Che should leave them as null in the Java config object. This way the command and the entrypoint properties won't be included in the serialized JSON object passed to the Docker API, and the mysql container will be started with the correct entrypoint and command.

In the second commit I modify the Java-MySQL stack to use the original mysql docker image.

What issues does this PR fix or reference?

Dockerfile for the codenvy/mysql image? #2991

Previous behavior

Stacks had to use the custom codenvy/mysql image in the Docker Compose definition.

New behavior

Stacks can use the official mysql image in the Docker Compose definition.

Signed-off-by: Kaloyan Raev kaloyan.r@zend.com

[codenvy-ci]

Can one of the admins verify this patch?

[benoitf]

ci-build

[codenvy-ci]

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1008/

[bmicklea]

@kaloyan-raev - we'd like to get this merged for M8 but there are conflicts, can you do a quick rebase?

[kaloyan-raev]

@bmicklea give me a few minutes.

[garagatyi]

-1 on current approach. Null value should not be returned as collection.

[garagatyi]

I believe it is responsibility of MachineProviderImpl of DockerConnector to workaround situation with empty collections.

[kaloyan-raev]

@garagatyi

-1 on current approach. Null value should not be returned as collection.

Why?
null and an empty collection are semantically different and masking null as an empty collection should be done only with a clear purpose. And it looks wrong at this particular place.

[benoitf]

I agree with @kaloyan-raev that if user has not set anything it should be null, while an empty value is empty collection

to workaround situation with empty collections.

There is no workaround, it's how it should work. Nothing entered, then it's null.

[kaloyan-raev]

Conflict resolved.

[JamesDrummond]

For crosslink: This PR effects #3204 .

[garagatyi]

Because we use that convention in our code. It makes methods that return collections free from NPE problems. It is very widespread in our code. Changed code was using that approach and personally I -1 on changing it to achieve what can be achieved in a different way.

[riuvshin]

Moved to M9

[bmicklea]

@kaloyan-raev - are you able to update this PR based on the feedback?

[kaloyan-raev]

@bmicklea No. The feedback by @garagatyi raises a code styling complain, but does not suggest a working alternative.

The MachineProviderImpl class is mentioned, but as far as I can see it actually assumes that the CheServiceImpl can return null. See: https://github.com/eclipse/che/blob/42272e4bd95c5b9b3c4fc20df5ef60685cac189f/plugins/plugin-docker/che-plugin-docker-machine/src/main/java/org/eclipse/che/plugin/docker/machine/MachineProviderImpl.java#L526-L527

[TylerJewell]

@garagatyi - there is no mention of such Collections best practices in the wiki: https://github.com/eclipse/che/wiki/Coding-Guidelines.

As such, @kaloyan-raev - +1 for merge. If we need to revisit our coding guidelines, we'll do so in the wiki, but not in this PR.

We are in ramp down for 5.0 and want to GA this month. This issue is a pre-req for this, so will you be able to rebase and merge? This issue is also a blocker for other issues that we have for 5.0 GA.

[kaloyan-raev]

Github does not show any conflicts for this PR, so I don't think it needs another rebase.

[TylerJewell]

You are a committer now, so the PR owner is responsible for merging, updating any docs, and cleaning up the branches. cough cough.

[kaloyan-raev]

OK :-)

I have to admit that I am not really sure when are the good days to merge things to master and when not. I am also not sure how it is decided for milestone assignments.

[TylerJewell]

The general rules for milestone assignments has been:

1. Product managers are able to designate groups of feature improvements for a particular milestone. We typically only track 1-2 milestones at a time. When a feature is designated for a milestone, then all associated bugs & tasks for that feature are included.

2. All severity/blocker issues are immediately placed into the current milestone, as they are considred blockers.

3. All regressions are labeled as severity/blocker until they are reviewed by PM to either keep that designation or moved to a different designation.

4. When we make a release, then technical leads can take any merge-ready PRs and add the next milestone to it, and merge at the beginning of the milestone.

And then along the way, when we do our reviews a couple times a week, if we identify other items that we think are good for the release, then a PM adds them in.

[kaloyan-raev]

Thanks!
It would be nice to have some "org chart" of the Che committers and mark those with the special roles and responsibilities. I already recognize some people, but I am still missing the overall picture.

[TylerJewell]

Yes, this would be good to add. I think we have product managers, maintainers, and committers. I'll work on something over the holidays.