fix stop mojo by taking container name pattern into account #1427
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
============================================
+ Coverage 61.89% 61.92% +0.03%
- Complexity 2160 2164 +4
============================================
Files 166 166
Lines 9537 9548 +11
Branches 1441 1442 +1
============================================
+ Hits 5903 5913 +10
- Misses 3119 3120 +1
Partials 515 515
|
|
This would be very useful for our team. Please consider its review. |
There was a problem hiding this comment.
In general I believe the use case that this PR is solving and that is described in the reference issue is valid, but I'm concerned about backwards compatibility which must be ensured.
Unfortunately I don't have enough time to dive deeper in this PR technically, but @rohanKanojia if you could give it tech review that would be cool.
|
|
||
| return keepOnlyLastIndexedContainer(containers, partiallyApplied); | ||
| String appliedContainerNamePattern = new FormatParameterReplacer(lookups).replace(containerNamePattern); |
There was a problem hiding this comment.
Is it correct that this changes the current behaviour in a non-backwards compatible way? (i.e. that all containers are stopped now by default, not the latest started)
If so we would need to be more careful to keep the old behaviour (at least for a certain deprecation period) and introduce this new behaviour either by a certain indication in the container name pattern or if this is not possible, with a new flag for docker:stop.
There was a problem hiding this comment.
Yes, that's true. I have changed it so that the new approach is only applied if the container name pattern doesn't contain an index placeholder. This way it should be backward compatible again.
|
btw, I'm also not sure why only the container name with the highest index is used (when the container pattern includes a |
|
Can we get #1410 in first and then check out how this fit to this PR ? (guess there might be a conflict) |
|
@j3t: Hello, Thanks for updating this PR. Somehow commit history seems to be messed up. Could you please take a look into it? Let's try to get it reviewed and merged soon. |
j3t
force-pushed
the
fix_1168_more_robust_stop_procedure
branch
from
71e3cc0 to
ac614a2
Compare
…ainers are stopped
|
Hi @rohanKanojia, is there any chance this gets included in the next release? What is missing? How can I help? |
|
Kudos, SonarCloud Quality Gate passed! |
|
Looks good to me. Let's get that merged. |
|
@j3t thanks a ton ! Sorry for the delay, I hope we can make a release asap. |
|
@j3t: I'm sorry for the late reply. I was on PTO. Do you need a new patch release with this fix? Or are you okay with waiting for the next minor release? |
No worries, the next minor release is sufficient I guess. |
Fixes #1168