Add a "clean" command to drupalci to clean up and remove Docker containers.

Currently src/DrupalCI/Console/Command/CleanCommand.php has a stub that just dumps the args, replace with real code.

* Create a follow-up issue to use docker-php.

CommentFileSizeAuthor
#8 clean-2489404-8.patch3.71 KBgchaix
#5 clean-2489404-5.patch3.66 KBgchaix
#2 clean-2489404-2.patch3.62 KBgchaix

Comments

gchaix’s picture

gchaix commented
Issue summary: View changes
gchaix’s picture

gchaix commented
StatusFileSize
new clean-2489404-2.patch3.62 KB

An initial "drupalci clean" command. Adds a "--list" command to list all created containers. Currently does not discriminate what type of container or image to clean, it just shuts down and removes all created containers (but not images).

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Active » Needs work

Edit: the basic run down of the plugin makes sense, but I think this could use some clean up.

  1. +++ b/src/DrupalCI/Console/Command/CleanCommand.php
@@ -23,21 +25,76 @@ class CleanCommand extends DrupalCICommandBase {
+    } else {
...
+      } else {
...
+    } else {
...
+    } else {

    if/else code blocks should be on a new line per coding standards. 

  2. +++ b/src/DrupalCI/Console/Command/CleanCommand.php
@@ -23,21 +25,76 @@ class CleanCommand extends DrupalCICommandBase {
+    // get list of running containers
+    exec('docker ps -q', $runningContainers);
+    // get list of created containers
+    exec('docker ps -a -q', $createdContainers);

    Is this just to display the containers before deleting them. I think that may generate a lot of console output.

    I wonder if a summary of the containers that were removed might be easier to read. Is it possible to get the output instead?

    Also, I think that there is a Container command that may be better to use here instead of PHP exec() - docker-php

    Maybe Docker\Manager\ContainerManager ?

gchaix’s picture

gchaix commented

I'll clean up the if/else blocks. Silly oversight. I should know better.

The second point regarding getting the list of containers - the code quoted there isn't what generates the output. It's grabbing the list of containers to decide what docker commands to execute. The actual console output is later. I'll suppress it, as it's really only useful for debugging, and resubmit.

gchaix’s picture

gchaix commented
StatusFileSize
new clean-2489404-5.patch3.66 KB

Cleaned up per coding standards, suppress debug output.

gchaix’s picture

gchaix commented
Status: Needs work » Needs review

Bah, forgot to change to "needs review".

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Needs work 
  1. +++ b/src/DrupalCI/Console/Command/CleanCommand.php
@@ -23,21 +25,80 @@ class CleanCommand extends DrupalCICommandBase {
+   * (#inheritdoc)
...
+   * (#inheritdoc)

    Shoot. I didn't catch this before, but (#inheritdoc) -> {@inheritdoc} typo. :-) 

  2. +++ b/src/DrupalCI/Console/Command/CleanCommand.php
@@ -23,21 +25,80 @@ class CleanCommand extends DrupalCICommandBase {
+   */

    Would an @todo referencing the comments above or a follow-up issue to clean up the clean task to use docker-php be relevant?

    Maybe we just create a follow-up task if approved / RTBC. I probably won't be able to test this until the weekend. 

  3. +++ b/src/DrupalCI/Console/Command/CleanCommand.php
@@ -23,21 +25,80 @@ class CleanCommand extends DrupalCICommandBase {
+      } ¶
...
+    } ¶
...
+    } ¶

    Looks like this patch added some trailing whitespace.

    In vim, though I guess you're probably using a GUI editor: :%s/\s\+$//g. I use that snippet all of the time.

gchaix’s picture

gchaix commented
Status: Needs work » Needs review
StatusFileSize
new clean-2489404-8.patch3.71 KB

Fixed #inheritdoc and trailing whitespace. Added TODO for replacing PHP exec() with docker-php.

gchaix’s picture

gchaix commented

Is this still relevant? I was going to look at replacing the PHP exec() stuff with container commands, but won't bother if we're not going to use it.

hestenet’s picture

hestenet
He/Him
Location Portland, OR 🇺🇸
commented
Parent issue: » #2534132: Disable Legacy Testbots and use drupalCI as our testing infrastructure
isntall’s picture

isntall commented

Yes i think this is very relevant.
It is missing some checks to make sure that only drupalci containers are being cleaned.

  • isntall committed 18e78c4 on 2489404-drupalci-clean authored by gchaix 
    Issue #2489404 by gchaix: drupalci clean
  • isntall committed 40c0dd4 on 2489404-drupalci-clean
    Issue #2489404 by isntall: drupalci clean
isntall’s picture

isntall commented

This command took a turn into the explicit.

The command is now
drupalci docker-rm <web|db|containers>

  • web - remove web containers
  • db - remove db containers
  • containers - remove all DCI containers

  • isntall committed 8aa8c98 on 2489404-drupalci-clean 
    Issue #2489404 by isntall: comment cleanup
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

LGTM

isntall’s picture

isntall commented
Status: Reviewed & tested by the community » Fixed

These changes have been committed to the production branch and the dispatcher.dci.o jobs have been updated.

isntall’s picture

isntall commented

The drupalci docker-rm containers command runs at the start of every new job, removing all containers associated with drupalci.
This gives a clean state to run the tests from and also gets around the issue where some jobs are still running even though jenkins thinks they are not.

drupalci docker-rm only stops and removes drupalci containers, it does not remove the data created by the web containers. though when a new database container is started, the database overwrites what was in the db directory.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.