- restore ability to use a random container name

[jgangemi]

this addresses #1352 and restores the ability to get a random container name chosen by the docker engine by specifying %r as the container pattern.

the loss of this functionality from previous plugin versions is a regression for me. none of the current naming schemes allows me to run multiple PRs for a project on the same instance b/c i get container naming conflicts.

@rhuss 👋

[codecov]

Codecov Report

Merging #1410 (569a2c7) into master (b2f287d) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@            Coverage Diff            @@
##             master    #1410   +/-   ##
=========================================
  Coverage     59.03%   59.03%           
- Complexity     1981     1985    +4     
=========================================
  Files           162      162           
  Lines          9019     9023    +4     
  Branches       1363     1364    +1     
=========================================
+ Hits           5324     5327    +3     
  Misses         3205     3205           
- Partials        490      491    +1     

Impacted Files	Coverage Δ	Complexity Δ
...fabric8/maven/docker/util/ContainerNamingUtil.java	80.00% <91.66%> (-0.36%)	22.00 <3.00> (+4.00)	⬇️

[rhuss]

Hi @jgangemi, nice to meet you again ;-) Hope you are doing well !

See my comment below, but I really wonder, which feature worked for you in the past and is now broken. Did we already had a %r placeholder ? Sorry for my ignorance, but I'm working on this plugin only in intervals, and its really in kind of a maintenance mode (i don't use it actively, nor do I currently program in Java at all ;-)

[rhuss]

Is it required that %r is given as a single argument ? What happens to any additional character added to the pattern ? Are they just ignored ? If so, then %r is not the same like the other "placeholders" as it just indicates a different behaviour that e.g. no name should be set at all. While the feature to have random containers names is desirable, its probably not a good fit to introduce it via a placeholder pattern.

[rhuss]

Maybe you could just check, whether the patterns contains a single "%r" with no extra chars. If this is the case, don't set a name. If the pattern contains a %r with additional characters, then throw an error. So I just would add it as a special case, and don't try to push it into the regular placeholder handling.

[jgangemi]

prior to this #1004 being merged, if i did not specify any options for the container name it would be started with a random one from the docker engine. since being merged, i can find no other way to get a random container name, hence this PR.

what if the naming pattern was intentionally assigned null, eg: <namingPattern>null</namingPattern> or none or any other thing that isn't specifically a pattern to avoid any confusion.

as always, i'm happy to make whatever changes are necessary to get this functionality back. it seems this is the general code area the change needs to occur in either way.

[rhuss]

what if the naming pattern was intentionally assigned null, eg: <namingPattern>null</namingPattern> or none or any other thing that isn't specifically a pattern to avoid any confusion.

Well, what if you want to call your container null or none ? (a bit constructed, sure, but not entirely out of scope). So maybe we really do it like this:

• Use a %r as you suggest, but check that if given it must be the only content of <namingPattern>
• If there are additional chars in addition to %r, then throw an error.

I think, this is probably the best way to reintroduce. In retrospective, it would have been better to let the default be an empty name, not the one that is currently chosen.

[jgangemi]

In retrospective, it would have been better to let the default be an empty name, not the one that is currently chosen.

i agree. i'm quite surprised this hasn't been a bigger issue b/c it prevents you from doing multiple builds in a CI environment. having just re-read the docs, i see this:

The following example is using a container name pattern of %n-%i which is also the default. Given an image fabric8io/dmp-sample-jolokia:latest, then during mvn docker:start a container with the name dmp-sample-jolokia-1 is first tried. If there is already a container with this name, then dmp-sample-jolokia-2 is the second attempt. This goes on until a "free" name is found.

but that doesn't seem to work the current version if the plugin i am using - i'll try upgrading to see if it's working in the latest.

either way, i would still prefer to have random name functionality so i'll make the suggested changes.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

95.7% Coverage
0.0% Duplication

[jgangemi]

any chance of seeing movement on this? right now there is still some bug that exists w/ the name-number container pattern where things aren't shutdown or skipped over properly.

[rhuss]

Sorry just back for the weekend (tbh, I can't spend more than once every x months on this plugin anymore, but working on to hand it over to @rohanKanojia and anybody else would like to continue to work here).

The more I read through the history of this issue, the more I think we should revert back to a default of a random container name (as it is also used by Docker itself when no name is specified). In retrospective, it was indeed an error to change the default value

I've seen that @rohanKanojia already added the desired change, so happy to merge this PR now.

[rhuss]

Suggested change

	| A random container name choosen by the docker engine.
	| An empty name that triggers are random container name chosen by the docker engine. If using `%r` this must be the only content of this pattern. Otherwise, an error is thrown.

[rhuss]

Suggested change

	| *%r*
	| *%e*

Can we use something different than %r please ? Because in the future we might also to add %r as a placeholder for a client-side random number, that we add to some pattern.

[rhuss]

wait a second with merging, just want to improve it a bit an change the replacement parameter name.

[rhuss]

@rohanKanojia could you give my update a quick look ? If it looks ok, happy to merge it now.

[jgangemi]

Sorry just back for the weekend (tbh, I can't spend more than once every x months on this plugin anymore, but working on to hand it over to @rohanKanojia and anybody else would like to continue to work here).

thanks @rhuss!! i'm always happy to help w/ things when time allows as well. i use this plugin heavily in a lot of places, so best it stays around. :)