Initial implementation of containerNamePattern #1004
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1004 +/- ##
============================================
+ Coverage 51.77% 51.86% +0.08%
- Complexity 1398 1413 +15
============================================
Files 147 148 +1
Lines 7573 7643 +70
Branches 1143 1151 +8
============================================
+ Hits 3921 3964 +43
- Misses 3278 3296 +18
- Partials 374 383 +9
|
|
Thanks a lot ! I will have a look asap, however quite busy this week (even busier than usual), so might take a bit ... |
There was a problem hiding this comment.
Looks good in general except that I think the stop logic wont work as before (see inline comment).
We should go back to the original logic which evaluate labels which has been attached when a container has been started. Thats easier and we dont have to hunt for the name.
I did some refactorings/simplifications and pushed to your branch. 'hope you don't mind.
As I'm going to make a release today, we wont get this in right now, but for the next release.
I also plan to switch to Java 8, so the next releaes will be probably something like 0.30
| String key = pomLabel.getKey(); | ||
| Map<String, String> labels = container.getLabels(); | ||
| return labels.containsKey(key) && pomLabel.equals(new PomLabel(labels.get(key))); | ||
| return container.getName().equals(namingConfiguration.calculateLastContainerName()); |
There was a problem hiding this comment.
This wont work as your build timestamp / index might be different than at the time when docker:start was called. Remember, this method is for checking whether a container should be stopped when mvn docker:stop has been called after one or more docker:start and it is supposed to stop all of them. Therefore the labels are examined whether it has been started by this particular pom. So we should keep the logic around excaming the marker labels on the containers. See also the documentation at https://dmp.fabric8.io/#docker:stop
|
Many Thanks btw, and sorry for the delay ;-) |
|
@rhuss sure, no worries. Didn't know this was there otherwise I'd have used it. So you changed what you commented on already, is there anything else that needs to be done? |
|
Sorry, overlooked your question ;-( I will relook at it and probably going to merge towards the end of the week. |
* Moved most logic into an utility class with a single static method * Moved calculation of the names to the service layer, Mojos should stay as slim as possible. * Reused already existing "FormatParameterReplacer" (and introduced lambda for the first time)
| @@ -188,7 +188,7 @@ private RunImageConfiguration extractRunConfiguration(ImageConfiguration fromCon | |||
| .links(valueProvider.getList(LINKS, config == null ? null : config.getLinks())) | |||
| .memory(valueProvider.getLong(MEMORY, config == null ? null : config.getMemory())) | |||
| .memorySwap(valueProvider.getLong(MEMORY_SWAP, config == null ? null : config.getMemorySwap())) | |||
| .namingStrategy(valueProvider.getString(NAMING_STRATEGY, config == null || config.getNamingStrategyRaw() == null ? null : config.getNamingStrategyRaw().name())) | |||
| .namingStrategy(valueProvider.getString(NAMING_STRATEGY, config == null || config.getNamingStrategy() == null ? null : config.getNamingStrategy().name())) | |||
There was a problem hiding this comment.
| if (runConfig.getContainerNamePattern() != null) { | ||
| return runConfig.getContainerNamePattern(); | ||
| } | ||
| if (runConfig.getNamingStrategy() == RunImageConfiguration.NamingStrategy.alias) { |
There was a problem hiding this comment.
| * @deprecated use {@link #getContainerNamePattern} instead | ||
| */ | ||
| @Deprecated | ||
| public NamingStrategy getNamingStrategy() { |
There was a problem hiding this comment.
| // Naming scheme for how to name container | ||
| @Deprecated // for backward compatibility, us containerNamePattern instead | ||
| public enum NamingStrategy { |
There was a problem hiding this comment.
| /** | ||
| * @deprecated use {@link #containerNamePattern} instead | ||
| */ | ||
| @Deprecated | ||
| public Builder namingStrategy(NamingStrategy namingStrategy) { |
There was a problem hiding this comment.
| @Parameter | ||
| @Deprecated | ||
| private NamingStrategy namingStrategy; |
There was a problem hiding this comment.
| /** | ||
| * @deprecated use {@link #containerNamePattern} instead | ||
| */ | ||
| @Deprecated | ||
| public Builder namingStrategy(String namingStrategy) { |
There was a problem hiding this comment.
|
SonarQube analysis reported 11 issues
Watch the comments in this conversation to review them. |
Replacement of #944
In general I'm quite happy, but even though the tests work and I tried I'm not so sure if there are not any naming configurations that do not work with watchers and the shutdown code.