Page MenuHomePhabricator

We need to run tests on the PHP code in the deploy repo
Closed, ResolvedPublic

Description

We caused a production fault because our PHP vendor directory was missing some files. :(

Event Timeline

cscott created this task.Dec 2 2019, 6:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 2 2019, 6:00 PM
Arlolra claimed this task.Dec 2 2019, 7:19 PM
ssastry moved this task from Backlog to Testing / QA on the Parsoid-PHP board.Dec 2 2019, 7:27 PM

Change 554140 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: [Parsoid deploy] Add PHP72 tests as experimental

https://gerrit.wikimedia.org/r/554140

Change 554140 merged by jenkins-bot:
[integration/config@master] layout: [Parsoid deploy] Add PHP72 tests as experimental

https://gerrit.wikimedia.org/r/554140

I've quickly added the parsoidsvc-composer-package-php72-docker job as experimental, but that's insufficient because (a) using the sub-repo doesn't quite work, and (b) that's a remote-fetch composer job which won't test the particular issue in question here.

Legoktm added a subscriber: Legoktm.Dec 2 2019, 8:46 PM

IIRC we should be able to use a job like mediawiki/vendor which uses the existing dependencies, and adds the development dependencies on top. I forgot how exactly that's implemented now and whether it's quibble specific.

Arlolra removed Arlolra as the assignee of this task.Dec 2 2019, 10:04 PM
Arlolra added a subscriber: Arlolra.
Jdforrester-WMF triaged this task as High priority.Jan 14 2020, 7:15 PM

Oh, totally forgot about this. Not good. Nearing on UBN.

Change 564764 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/services/parsoid/deploy@master] Provide a composer test entry point for pre-prod final verification

https://gerrit.wikimedia.org/r/564764

Change 564767 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] [jjb] Provide parsoidsvc-deploy-composer-php72-docker to test

https://gerrit.wikimedia.org/r/564767

Change 564768 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: [parsoid/deploy] Wire up parsoidsvc-deploy-composer-php72-docker as experimental

https://gerrit.wikimedia.org/r/564768

Change 564769 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: [parsoid/deploy] Make parsoidsvc-deploy-composer-php72-docker voting

https://gerrit.wikimedia.org/r/564769

Change 564767 merged by jenkins-bot:
[integration/config@master] [jjb] Provide parsoidsvc-deploy-composer-php72-docker to test

https://gerrit.wikimedia.org/r/564767

Change 564768 merged by jenkins-bot:
[integration/config@master] layout: [parsoid/deploy] Add parsoidsvc-deploy-composer-testonly-php72-docker

https://gerrit.wikimedia.org/r/564768

Change 564764 merged by jenkins-bot:
[mediawiki/services/parsoid/deploy@master] Provide a composer test entry point for pre-prod final verification

https://gerrit.wikimedia.org/r/564764

So, we got so close on this, but it doesn't work.

composer test in parsoid-deploy runs parsoid's bin/toolcheck.php.sh, which loads parse.php, which loads Maintenance.php, which tries to load parsoid's autoload.php from its vendor directory (which is empty in the parsoid-deploy sub-module).

Manually hacking the Maintenance.php file to use parsoid-deploy's vendor directory then throws PHP Fatal error: Uncaught Error: Class 'Monolog\Logger' not found in /src/src/src/Config/Api/SiteConfig.php:117

diff --git a/composer.json b/composer.json
index 3faae9a20..df6195d45 100644
--- a/composer.json
+++ b/composer.json
@@ -29,6 +29,7 @@
                "wikimedia/scoped-callback": "3.0.0",
                "wikimedia/wikipeg": "2.0.3",
                "wikimedia/zest-css": "1.1.2",
+               "monolog/monolog": "~1.25.2",
                "ext-dom": "*",
                "ext-json": "*"
        },
@@ -36,7 +37,6 @@
                "jakub-onderka/php-parallel-lint": "1.0.0",
                "jakub-onderka/php-console-highlighter": "0.4.0",
                "jakub-onderka/php-console-color": "0.2",
-               "monolog/monolog": "~1.25.2",
                "mediawiki/mediawiki-codesniffer": "28.0.0",
                "mediawiki/mediawiki-phan-config": "0.9.0",
                "mediawiki/minus-x": "0.3.2",
@@ -49,14 +49,14 @@
        },
        "autoload": {
                "psr-4": {
-                       "Parsoid\\": ["src/placeholder", "src"]
+                       "Parsoid\\": ["src/placeholder", "src"],
+                       "Parsoid\\Tools\\": "tools/",
+                       "Parsoid\\Tests\\": "tests/mocks/"
                }
        },
        "autoload-dev": {
                "psr-4": {
-                       "Parsoid\\Tests\\": "tests/mocks/",
                        "Parsoid\\Tests\\ParserTests\\": "tests/ParserTests",
-                       "Parsoid\\Tools\\": "tools/",
                        "Test\\": "tests/phpunit/"
                }
        },

Alternatively, could we just load monolog on-demand? I think there was an outstanding issue where we didn't actually want command-line invocations of parsoid to be logging to the production servers (or anywhere) by default. If we make no-logging (or log-to-stderr) the default command-line invocation, we wouldn't have to move monolog to prod dependencies. There should be no reason for mocks to be in the prod autoload, either. It would be nice if we could arrange things such that tools/ isn't needed: the entry point script can be in tools/ as long as it doesn't then depend on anything else in tools (at least not w/o manually adding that path to the autoload path).

Just trying to reduce our security footprint by keeping unnecessary code out of the prod autoload path. (Granted, production MW uses monolog itself, so that's not really opening up our security footprint, keeping monolog out is more about "no non-local logging by default" for CLI scripts.)

Change 567159 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/services/parsoid@master] Maintenance: Point in a different direction so this passes in /deploy if needed

https://gerrit.wikimedia.org/r/567159

Change 564815 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/services/parsoid/deploy@master] More fixes to make composer test work

https://gerrit.wikimedia.org/r/564815

Change 567159 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Maintenance: Point in a different direction so this passes in /deploy if needed

https://gerrit.wikimedia.org/r/567159

Change 564769 merged by jenkins-bot:
[integration/config@master] layout: [parsoid/deploy] Make PHP tests voting

https://gerrit.wikimedia.org/r/564769

Mentioned in SAL (#wikimedia-releng) [2020-02-10T23:42:27Z] <James_F> Zuul: [parsoid/deploy] Make PHP tests voting T239642

Change 564815 merged by jenkins-bot:
[mediawiki/services/parsoid/deploy@master] More fixes to make composer test work

https://gerrit.wikimedia.org/r/564815

Jdforrester-WMF closed this task as Resolved.Tue, Feb 11, 12:16 AM