resolves issue-87 #333
resolves issue-87 #333
Conversation
This env variable WHALE_DMP_LABEL is used to mark containers to belong to a certain build so that can be recognized when calling 'docker:stop' #87
Related to #87, still missing: Query for labels and stopping based on maven coordinates.
Fixed small typo
Update README.md
- docker:stop no longer acts like a sledgehammer and stops containers it shouldn't (unless asked) Signed-off-by: Jae Gangemi <[email protected]>
|
i'd also really appreciate it if you could prioritize looking this over so i can address any feedback, etc as it's crucial for some stuff going on at the day job. |
|
I'll try to look at the asap but I'm afraid that won't be before mid of next week, because of release deadline and preparing a talk for a conference next week. After that I'll have much more time to spend on the plugin in order to push it forward. Sorry for that. In the meantime, could you check this PR again ? Seems that quite some old commits were included (including mine). Doesn't feel right ... |
|
i'll take a look. i pulled in your branch and then re-based the latest integration branch on top of that, but maybe i did something wrong. no worries about not being able to get to this. we can use a snaphot version of the plugin that i can push up. |
|
i think those changes are correct. don't forget you did this work on a separate branch that you pushed and i pulled. these are the steps i just followed ( from my
and i get the same results as above. |
| "' has not the format <group>:<artifact>:<version>[:<runId>]"); | ||
| } | ||
| mavenCoordinates = parts[0] + ":" + parts[1] + ":" + parts[2]; | ||
| runId = parts.length == 4 ? parts[3] : null; |
There was a problem hiding this comment.
Is there a reason why runId is here null by default ? That's inconsistent to the constructor with no runId which initializes the run id to an UUID.
There was a problem hiding this comment.
i'm going to have to defer that back you to you - looking at your branch, it would seem you wrote that code. :)
i had to alter matches to allow comparison against the runId to be optional b/c there's no way the plugin can know that value when it's called directly via docker:stop, so in order to stop all the containers, we can only match on the coordinates.
as an aside, i'm not sure the runId is needed on a certain level b/c the plugin already only stops/removes containers it started during integration tests and/or if there is some problem during initial start up. having said that, i still think it's a good idea to tag them.
There was a problem hiding this comment.
;-P well, was quite some time ago (and I'm getting old :) ....
Thinking again, I also think that a runId is probably not needed for this use case but could be helpful to identify all containers which has been started together with a single 'mvn docker:start' call. But I wonder whether we shouldn't add it as an extra Label (if needed, so not now) ? I'll remove it for now to make things more straight forward.
* Label is renamed to 'dmp.coordinates' * `runId` removed from label * renamed property for stopping all containers from the images to 'docker.allContainers' so that a 'mvn docker:stop -Ddocker.allContainers' will reproduce the old behaviour. * Some minor refactorings See also #87.
it's a glorious day! :)
couple of things...
runIdto work, but i had to add an option to ignore it when callingdocker:stopfrom a standalone context. i had already made a change (i forget when) that fixed this for containers that were started/stopped during the same build run. i didn't remove any of that code, i just added an option to ignore the run id.2). i bumped things to
0.14-0-SNAPSHOTb/c this required the docker api to be rev-ed.v1.18is still part of docker 1.6, so perhaps it can stay at0.13.7-SNAPSHOT.there is still the option to make the plugin work like it did before using
docker.sledgehammer(and i really hope we can keep that name)i did not bother to account for containers that may have been created using an older version of the plugin and have upgraded to whatever version this goes out as. it didn't seem worth the extra work when the containers can be stopped using
-Ddocker.sledgehammerand then re-created w/docker:startand have the labels applied.