stage1/docker-php committed a significant refactoring a couple of weeks ago. This refactoring provides not only the original container and image manager, but also new exec, volume, network, and miscellaneous (i.e. all other) manager objects. This release also removes all of the "there is no stable version of docker-php" text from their documentation.

Assuming the implementation is complete, upgrading docker-php should help remove a lot of complexity within the DrupalCI codebase. That said, it is a major refactoring, and will require extensive testing.

CommentFileSizeAuthor
#19 Conversion_to_guzzlehttp_tests.patch5.68 KBjthorson
#19 Conversion_to_guzzlehttp_code.patch5.11 KBjthorson
#3 upgrade_docker.patch12.31 KBjthorson

Comments

jthorson created an issue. See original summary.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2656446: Evaluate composer.json needs

Related: #2656446: Evaluate composer.json needs

Our composer.json says:

"stage1/docker-php": "dev-ryanaslett-rawstreams-1"

Ping @Mixologic. :-)

jthorson’s picture

jthorson commented
Status: Active » Needs review
StatusFileSize
new upgrade_docker.patch12.31 KB

Updated and changes pushed on the 2656480-upgrade-docker-php branch.

We'll need to ensure that the stream timeout patch from Ryan's branch doesn't sneak back in with this change ... but the new docker-php rewrite appears to be using much more recent packages (and all in all, is much nicer to work with once you're familiar with it!)

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Postponed

This is a prime candidate for "make these changes after we have tests in place".

Otherwise we're also missing the actual change to the composer.json to make this use the updated lib.

Edit: saw that was in the branch, just not this patch.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

The 'after we have tests' issue: #2544340: Make the tests pass

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Postponed » Needs work

#2544340: Make the tests pass is fixed, so un-postponing here.

jthorson’s picture

jthorson commented
Status: Needs work » Needs review

Unless you have feedback, resetting to the original issue status.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

So, lets get https://www.drupal.org/node/2676854 in (provided it works, I've only done the R part, not the T part of RTBC), and add tests that prove that docker works fine. Then we add this branch in and prove it still works fine and then we can √ this off the todo list.

jthorson’s picture

jthorson commented

Does #2676854-14: [meta] Write functional tests to prevent regressions satisfy your need for a docker test first?

Going from stage1/docker-php to docker-php/docker-php is a complete rewrite and restructuring of the library, along with completely different class structure and methods. Even right off the start, the very first Client::createWithEnv method we use has been renamed to Client::createFromEnv.

We can write tests against the old library, but the changes are such that very few of those tests will actually carry forward without needing a full rewrite. I'd go so far as to suggest that, other than AssertInstanceOf('ImageManager') and AssertInstanceOf('ContainerManager'), everything else will require a rewrite of the tests virtually from scratch. Once again, I can't see the value in writing new tests against the old codebase, when we're going to need to rewrite those tests ANYWAY when we move to the new. We're not gaining any efficiencies here ... we're just generating extra work on a project that's already falling further and further behind. (We've got 5 month old feature development still hanging around, languishing in the queue, waiting on reviews.)

Sorry that I don't agree with you here ... but building out a functional test with a fixed set of assumptions is no different than manually executing a functional test with a fixed set of assumptions. Sure, it would be great to automate it, make it reuseable, and make it repeatable ... but writing those tests now (and rewriting them to the new docker-php API later) does not give any greater test coverage than the manual execution that I've already ran this stuff through - I simply don't see the extra value in holding back this work for weeks to wait for in-code testing, just to save a few hours of manual testing which has already been performed. And the best we can do right now is functional testing, until the code cleanup and decoupling which is waiting in the queue has actually been committed ... I'd rather we spend that effort on developing unit tests.

Unfortunately, I also recognize that I'm fighting against strongly held emotional convictions regarding purity of process, based on belief and opinion ... which is a battle that is virtually impossible to win using logical judgements based on fact. But if you look at the pros and cons of postponing this on the grounds of needing test coverage, it becomes pretty clear that we're creating unnecessary work while stifling innovation ... and neither of those does us any favours, here.

  • jthorson committed 5c87967 on 2656480-upgrade-docker-php 
    Issue #2656480 by jthorson: Conversion to use guzzlehttp instead of...
jthorson’s picture

jthorson commented

Mile23: I got a bunch of the tests converted ... can I get you to take a look at the two tests which are still failing on this branch?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: Unassigned » mile23

Sure. Will poke around.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

So after a ton of rebasing from dev and trying to make things work I get this:

$ ./bin/phpunit 
PHPUnit 3.7.38 by Sebastian Bergmann.

Configuration read from /home/vagrant/drupalci_testbot/phpunit.xml

.............................PHP Fatal error:  Class 'Docker\Http\DockerClient' not found in /home/vagrant/drupalci_testbot/src/DrupalCI/Providers/DockerServiceProvider.php on line 22

Pushed rebased branch.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Please don't ask me to work on a branch and then push changes before I'm unassigned.

Thanks.

jthorson’s picture

jthorson commented 
$ git log
commit 5c8796786247694a4fd44b6593e4cb4ca544235f
Author: Jeremy Thorson <jthorson@sasktel.net>
Date:   Sun Mar 6 11:59:54 2016 -0600

    Issue #2656480 by jthorson: Conversion to use guzzlehttp instead of guzzle (in progress)

commit d92b5c23316021408586a33330949caf4d346482
Merge: d2f1a9e 666ca7f
Author: Jeremy Thorson <jthorson@sasktel.net>
Date:   Sat Mar 5 17:15:51 2016 -0600

    Merge branch 'dev' into 2656480-upgrade-docker-php

Why did it need rebasing?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: mile23 » Unassigned
jthorson’s picture

jthorson commented

No idea what you're talking about. I haven't touched the branch. Did you forget to pull?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Nope.

Did you forget to pull dev before you merged it? :-)

jthorson’s picture

jthorson commented
StatusFileSize
new Conversion_to_guzzlehttp_code.patch5.11 KB
new Conversion_to_guzzlehttp_tests.patch5.68 KB

If you're getting that error, you rebased my work out of the branch.

jthorson’s picture

jthorson commented

Last dev commit: Friday
Branch Merge: Saturday
My last commit: Sunday at 11:59
My push: Sunday at 12:03

My ask for help: Sunday at 12:04

I'm still not sure what happened there ...

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: Unassigned » mile23

OK, so one git branch -D/git checkout -b later....

jthorson’s picture

jthorson commented

Locally, I've got it running phpunit without Fatals, throwing fails on FetchTest and ContainerCommandTest.

jthorson’s picture

jthorson commented

Ah ... cgit not reflecting the push!

jthorson’s picture

jthorson commented

IRC?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

PullCommandTest says this:

1) DrupalCI\Tests\Console\Command\PullCommandTest::testPull
Run command did not throw exception or display status info.

/home/vagrant/drupalci_testbot/tests/DrupalCI/Tests/Console/Command/PullCommandTest.php:33

Replicating the test in the CLI I see this:

$ ./drupalci pull foof
Executing pull ...
Pulling foof:latest container
Pull Complete.

So that tells me that whatever changes were made here don't throw an exception or notify you that the container doesn't exist.

Does the new library throw a helpful exception we are inadvertently catching, or give us some other way to check whether there was a fail?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: mile23 » Unassigned
jthorson’s picture

jthorson commented

It used to throw an exception ... now it returns chunk-encoded json. I'm not sure how to go about dealing with that.

The ContainerTestCommand test is a little closer to working (i.e. probably more debuggable) ... I was thinking someone with more experience might be understand what it's complaining about, there.

Thanks!

jthorson’s picture

jthorson commented

Currently blocked on errors in docker-php. Needs resolution of:

https://github.com/docker-php/docker-php/issues/176
or
https://github.com/docker-php/docker-php/issues/180

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Component: Code » Testrunner Codebase
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Assigned: Unassigned » Mixologic

  • Mixologic committed 05daa16 on 2656480-upgrade-docker-php 
    Issue #2656480: Updates environment base to validate properly
  • Mixologic committed 2644715 on 2656480-upgrade-docker-php
    Issue #2656480: Upgrades the docker pull command. Its very ugly, but...
  • Mixologic committed 3702307 on 2656480-upgrade-docker-php
    Issue #2656480: updates composer.json/composer.lock again
  • Mixologic committed 494adc7 on 2656480-upgrade-docker-php
    Issue #2656480: Updates use statements
  • Mixologic committed 8d35ccc on 2656480-upgrade-docker-php
    Issue #2656480: updates the docker api version in vagrant
  • Mixologic committed ad17b6f on 2656480-upgrade-docker-php
    Issue #2656480: Updates the tests
  • Mixologic committed df386ed on 2656480-upgrade-docker-php
    Issue #2656480: Finds running service container properly
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: Mixologic » mile23

Tests pass, except for DrupalCI\Tests\Plugin\BuildSteps\generic\ContainerCommandTest, which has been marked as incomplete.

This change causes ./drupalci init to output super-fugly status messages like this: (It still works, just looks bad.)

Waiting
Waiting
 169/169 [============================] 100%Verifying Checksum
Download complete
 897/911 [===========================>]  98%Download complete
 1/7 [====>-----------------------]  14%Download complete
 22/22 [============================] 100%Verifying Checksum
Download complete
 7/7 [============================] 100%Verifying Checksum
Download complete
 327/327 [============================] 100%Verifying Checksum
Download complete
 254/254 [============================] 100%Verifying Checksum
Download complete
 390/390 [============================] 100%Verifying Checksum
Download complete
 41/74 [===============>------------]  55%Download complete
 329/329 [============================] 100%Verifying Checksum
Download complete
 256/256 [============================] 100%Verifying Checksum
Download complete
 391/391 [============================] 100%Verifying Checksum
Download complete
 74/74 [============================] 100%Verifying Checksum
Download complete
Extracting
Extracting
Extracting

In IRC @Mixologic says the overall goal is to move towards docker compose, which means ./drupalci init is essentially deprecated in favor of docker pull.

Since we need a test of the behavior of ContainerCommand, this is still 'needs work.'

Still not quite convinced about the drupalci init weird text, but it still works for now.

I'll write a test to cover ContainerCommand.

  • Mile23 committed c96aa93 on 2656480-upgrade-docker-php 
    Issue #2656480: Update ContainerCommandTest for new expectations.
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

OK, so here's what I did to ContainerCommandTest:

  • Substituted ClassName::class pattern for mock building.
  • Removed mock declarations that aren't required, specifically some method with() parameters.

This means that we're exercising the code in ContainerCommand::run(). We should follow-up with an issue to refactor some of these foreach() loops into separate methods and then we can successfully unit test with goals in mind.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs review » Fixed

All right, looked at the test, looked at everything else, and we've got new docker-php. Goods. so now we can php7, and we can start in on docker compose stuff. Merged into dev.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Assigned: mile23 » Unassigned

Status: Fixed » Closed (fixed)

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