πŸ‡ΊπŸ‡ΈUnited States @tedbow

Ithaca, NY, USA
Account created on 13 February 2008, about 16 years ago
  • Senior Software Engineer in OCTO at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

@fjgarlin thanks. Sorry forgot up the commit message with your name😬

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

CommittedπŸŽ‰

Thanks @dimitriskr!

Leaving this open so we can remove the changes not needed after #3421674: Add condition in .simpletest-db for mariadb β†’ is committed which hopefully will be soon.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Confirmed with @PhΓ©na Proxima

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Some more thoughts on Windows,

  1. @TravisCarden is now working on making the existing Rsync syncer that comes with Composer Stager work with Windows. He is not but seems like it is doable. Of course you would still have to install rsync somehow
  2. With the combo of 🌱 [policy, no patch] Drop support for Windows in production Needs review and ✨ Recommend DDEV as the default Drupal local development environment Active it does make me think it is not worth the effort to make Windows specific syncer
  3. I think the only case that leaves out is a Windows user who only has PHP and wants to just use the Quickstart script to tryout Drupal. I think with Quickstart Automatic Updates is probably not much use to them but in the future Project Browser might be also quickly try out some modules. I think we can deal with that when the time comes and decide at that point if it makes sense to make a Windows specific syncer, do πŸ“Œ Create a separate rsync shim Composer library for Composer Stager Active or recommend DDEV instead
πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

@drumm Ok thanks for the info
I have made a github issue for the Composer plugin https://github.com/php-tuf/composer-integration/issues/99

@drumm I asked in that issue but raising it here also. It seems that Composer has other ways to get the security advisories during `composer audit`. Given that what are the advantages of implementing this API. Are there other callers besides Composer itself?

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Removed all the changes related to `PackageManagerUpdateProcessor` because I think it is little more complicated. Lets handle that in πŸ“Œ Do not use the Update modules cache of UPdate XML so we can control how long it is cached Active where we decide to either always use our own processor or just take over the Update module's service.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I pushed up changes that remove all the changes to PackageManagerUpdateProcessor.php because I think they should be considered more and have other implications.

for instance they would change the last update time so if you used project browser to install a new module then package manager update the Last Updated time kept by the Update.

So imagine

  1. User 1 installs a module via project browser, last update time gets updated
  2. User 2 looks at Available Updates, sees no updates and that updates were just checked 1 minute ago(by user 1)

User 2 would seem safe to assume that the site is secure but the Update module might have actually last checked 6 hours ago and security release could have already been released.

I am sure we can solve this but don't know to for this issue.

I think we might actually need to rethink how we fetch updates in general because the update module generally was meant to fetch all updates at once.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Another wrinkle in this is when a created πŸ“Œ [policy, no patch] Use Update XML in Package manager to determine release support status Active assumed that the Composer metadata for drupal projects that Composer relies would be TUF protected because drupal.org is setting up this protection. This is true for the packages.json and the files like https://packages.drupal.org/files/packages/8/p2/drupal/webform.json(when TUF protection is in production)

but looking at ✨ Implement β€œlist security advisories” Packagist/Composer API Fixed This is wil be more information that drupal.org provides composer but I don't see how this could be TUF protected. For instance composer audit would make a request like https://packages.drupal.org/8/security-advisories?packages[]=drupal/tfa&packages[]=drupal/swiftmailer. Since packages in the URL query would be dynamic each site would send a different list based on the projects installed and get different results.

Right now when I run composer audit -vvv I see

Downloading https://packagist.org/api/security-advisories/
[200] https://packagist.org/api/security-advisories/
Found 2 security vulnerability advisories affecting 2 packages:

but no request for to <code>https://packages.drupal.org/8/security-advisories?... but I presume that would happen when ✨ Implement β€œlist security advisories” Packagist/Composer API Fixed is implemented completely. At that point I think this info can be TUF protected.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

To accomplish the plan in πŸ“Œ [policy, no patch] Use Update XML in Package manager to determine release support status Active we will probably need to the current issue.
Especially this part:

We should also check that the available versions returned by the Update XML, just the numbers not the metadata unknown to Composer, match the available versions as known to Composer (composer info -a).

If are going to check against versions know by release we can't rely an old version of the update xml or they might be out of sync.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

This looks good to me if the tests pass and we remove CI_DEBUG_SERVICES

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Re #8

@Warped I share some of your concerns I just don't think we have to solve all these problems in the Alpha version of the experimental module. We can adapt as we get alpha testers to make ultimate core stable experience as friction free as possible.
but I think we should be cautious about supporting more than we have to at the Alpha stage because I think we will have to support whatever we create for a while.

  1. I am not sure what percentage will use AU but because many sites will have a more complex deployment workflow they will probably not us it.

    .... more likely to set and forget, assume that a core service will just work, and have more difficulty when it doesn't.

    If AU is installed and automated/cron are updates are enabled then we give an error message on almost all admin pages(to privileged users) about any validation error that would cause AU not to work. Not meeting the requirement for rsync or whatever syncer we end up offering would fall into this category. We also would email the errors. Since sites will be relying on this for keeping the site secure from the start we have tried make sure the admin will also be notified if it is not going to work ahead of time.

    But the system requirement for is probably not something that is going to change very often. For instance if hosting offers rsync they probably will not remove it without giving very prominent warning to their customers.

  2. Should new users be presented with options that just work, instead of needing to immediately jump into the Drupal contrib system.

    Yes that is the goal. But it is hard for us to know ahead of time what capabilities hosting companies for sites that use AU will have. But luckily we have the experimental module process in core to figure this out.

    If in the process that 30% of sites won't have access to rsync then I would suspect that would need to support other syncers in core and not have those users have to go to contrib. 30% is just an example number I think it would a product decision what number would be acceptable. I don't think we could ever get to 100% as other requirements such as a writable file system(though there are options πŸ“Œ Add Symfony Console command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed ) will eliminate some sites.

  3. I agree. I think we will hopefully get testers in the experimental phase using that type of hosting and try to work during the experimental process to make their experience as good as can be, which might mean supporting more than rsync in core before stable.
  4. . Is there no other tool or method, besides trying to duplicate rsync in PHP, that would require less maintenance?

    I think the "syncers using base OS commands" mentioned in the summary will be that and we should consider them as option if rsync support in core is not enough

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Ok here are the 2 follow-ups

In https://github.com/php-tuf/drupal-project I added Create test module to enable TUF validation in Package Manager. This will mean if you are start a project with this project you will also get this test module you can turn on to point to the dev TUF repo.

The test module will need a change in the Package Manager contrib module to make the URL Composer repo it expects to be TUF protected changeable, so I created πŸ“Œ Make PhpTufValidator able to switch TUF protected repo URL for testing purposes Active .

Note, the change to the Package Manager contrib will automatically become part of the core merge request when the conversion is run after the issue is resolved. We will make sure the test module in https://github.com/php-tuf/drupal-project will handle working with the contrib module or the core version., for Composer and other reasons the machine of the module is different, automatic_updates in contrib and auto_updates in core

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Re the "Remaining Tasks" item

Decide which, if any, of the Proposed Resolution's possibilities for contrib need to be in place in order to make the above decision for core.

When determining whether and to what degree we should support hosting that does not have rsync and could not easily install it we have to consider it in relation to other requirements of Automatic Updates(actually Package Manager under the hood)

For example, if we determined that many restricted hosting setups did not have rsync but also determined that most of those hosting setups also had PHP's proc_open()(needed to invoked Composer) disabled then providing a non-rsync file syncer alternative would not actually help those users, unless we also solved the proc_open() problem for them.

Short of getting a lot of users to try the contrib module and somehow determining those users are also the likely Core AutoUpdates audience it will be hard to determine these problem before AutoUPdates is released as an experimental module in Drupal core.

It would be great to figure out how common rsync is but I think this is only part of the problem:
Because one likely scenario could be:

  1. We don’t know what % of users will not have rsync
  2. We do some effort to figure out what that % is
  3. We find out it is 20% :scream:
  4. We implement the 3 OS specific syncers before AutoUpdates is released in core
  5. That 20% now tries AutoUpdates in core
  6. We find out that 95% of the 20% can't use AU anyways because they also don’t meet some other requirement of AutoUpdates, for example proc_open()
  7. To use Automatic updates the 20% has to move to less restricted hosting which may actually have rsync

I personally think the best way to save ourselves from a ton of work, and the community from a long term maintenance nightmare is:

  1. Release the Core Alpha Experimental module with only rsync support
  2. (optional) also have some sort of Windows-only syncer option, maybe the already created PHP Syncer

We know that will stop some people from being successful but it is only Alpha and at that point we can ask users to provide a screenshot of their validation errors. This will tell us what other requirements they are not meeting. If almost everyone who has rsync missing also has some other basic/unsolvable validation error because they are on restricted hosting it should give us pause about creating the OS specific syncers because it would not actually help that many people.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

How can we run this against 10.1.x with the contrib version to make sure it still passes?

Ideally 3.0.x would be tested against both 10.2.x and 10.1.x but at the least I think we need be able to test before commit.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I am not clear why the drupalci test just now switched from test core 10.1 to 10.2.1. I guess 10.2.1 just came out?

So postponing this as I think 3.0.x 10.2.1 will actually fail. I am retesting 3.0.x first on that. Will see if it fails then open up a new issue if it does.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Using the Dev TUF endpoint as default

Re #39

I'm wondering given the DA has TUF on a dev endpoint, would it be possible (and if so, desirable) for us to point an alpha package manager to the dev endpoint?

I don't think this would be possible unless we are ok for Alpha testers to experience major disruption to using Composer.

Because the TUF protection provided by the system is enforced on the Composer Plugin level that means if TUF does not validate then probably most Composer operations will not work. This means even command line Composer operations would be halted not just Composer operation through Package Manager. This is by design if TUF doesn't validate then no operations against the Composer repository can be considered safe. Even operations that don't deal with drupal.org endpoint directly may behind the scenes make calls to drupal.org endpoint. This is Composer design choice not anything to do with our plugin, we just through errors for the Composer repositories that are TUF protected.

So if the DA TUF dev endpoint is really dev status and wouldn't be up and reliable as much as the production one then it could really cause headaches. Keep in mind this is for all Composer metadata not just TUF signing.

so for instance say the DA’s dev TUF metadata expired. or since it is dev say they just had to take if offline for a couple hours to redeploy it, this would mean those sites that were pointing to that endpoint could do no Composer operations until they updated their Composer.json files manually to point to the current production Composer repository and disabled TUF on the repo.

Test with Dev TUF endpoint

Although I think enabling the TUF protection via dev endpoint would be too disruptive for tester there is a way we could test TUF protection even if it is not on by default.

We have created https://github.com/php-tuf/drupal-project(we should probably rename) which is way to setup a drupal/recommended-project style project but have Composer pointed to dev endpoint instead of https://packages.drupal.org/8. A project create with this template will be TUF protected. On the Drupal level we have a PhpTufValidator that simply ensures the composer plugin is present and is configured to protect https://packages.drupal.org/* repos. PhpTufValidator is present in the current Package Manager module it just is not tagged with event_subscriber so its logic is not enforced. In tests we add the tag.

So either we could leave PhpTufValidator off for this testing, though the Composer plugin would still provide TUF protection or we could make the https://packages.drupal.org/8 URL overridable and have https://github.com/php-tuf/drupal-project depend on test module that would PhpTufValidator extend to protect the Dev endpoint and add the event_subscriber tag.

I think this method would allow people reviewing the module to test full TUF protection against the dev endpoint in the Alpha stage but then have most testers not have to worry about the unreliability of the TUF dev endpoint(though they obviously would not get TUF protection).

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Need to check if the tests now pass since πŸ› Only set content-length header in specific situations Fixed is fixed

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

We still to address @phenaproxima comments on the MR but for now πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work is critical

So we should get that working first

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

See MR comment

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Updating "Core Merge request planning"

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Marking as fixed becasuse the build tests passed on the last test run.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Assigning for any follow-up

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Committed this issue since it only affects the core converted version of the module.

@phenaproxima I left a question on the core MR for you. We can open up this issue again if we want to make this change to the unconverted version also or if you think the core converted version is the incorrect fix.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Committing because this only affects the converted module and the gitlab pipeline passed

This should fix the last build test fail(not ours) on πŸ“Œ [PP-1] Fix build tests for Automatic Updates core Gitlab Postponed

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Actually we do have another change that is applied to ✨ Add Alpha level Experimental Package Manager module Needs review that applies to both 10.1.x and 11.x so re-opening the issue because that is not blocked by [##3411110]

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I think the error we are getting

Fail Other phpunit-10.xml 0 Drupal\Tests\package_manager\Build\
PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
Bergmann and contributors.

Testing Drupal\Tests\package_manager\Build\PackageInstallTest
FF 2 / 2
(100%)

Time: 00:21.850, Memory: 12.00 MB

There were 2 failures:

1)
Drupal\Tests\package_manager\Build\PackageInstallTest::testPackageInstall
COMMAND: COMPOSER_MIRROR_PATH_REPOS=1 composer create-project
drupal/recommended-project project --stability dev --repository
'{"type":"path","url":"composer/Template/RecommendedProject"}'
OUTPUT:
ERROR: Creating a "drupal/recommended-project" project at "./project"
Installing drupal/recommended-project
(dev-59106db9170de044e42da516409f74c77b9992d0)
- Installing drupal/recommended-project
(dev-59106db9170de044e42da516409f74c77b9992d0): Mirroring from
composer/Template/RecommendedProject
Created project in
/tmp/build_workspace_21df0330a51859a5f17fd595455ba17boYGMnu/project
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

Problem 1
- Root composer.json requires drupal/core-recommended * -> satisfiable
by drupal/core-recommended[dev-59106db9170de044e42da516409f74c77b9992d0].
- drupal/core-recommended dev-59106db9170de044e42da516409f74c77b9992d0
requires drupal/core 11.x-dev -> found
drupal/core[dev-59106db9170de044e42da516409f74c77b9992d0] but it does not
match the constraint.

Failed asserting that 2 matches expected 0.

/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3411111/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php:307
/builds/issue/drupal-3411111/core/tests/Drupal/BuildTests/Framework/BuildTestBase.php:297
/builds/issue/drupal-3411111/core/modules/package_manager/tests/src/Build/TemplateProjectTestBase.php:501
/builds/issue/drupal-3411111/core/modules/package_manager/tests/src/Build/TemplateProjectTestBase.php:332
/builds/issue/drupal-3411111/core/modules/package_manager/tests/src/Build/PackageInstallTest.php:19
/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3411111/vendor/phpunit/phpunit/src/TextUI/Command.php:97

Points to the fact we need fix this first on this core issue.

I opened πŸ› Build tests fail on core merge requests Active in the contrib module for the changes we need there and will update the instructions there for pushing the changes here

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Ok the build tests fail which was expected. We will probably need to open up a contrib issue to fix the module. The we can run the conversion from the branch on core issue and push up to this branch. If we can get the build tests passing on both the drupalci in the contrib version and gitlab here then I think we can call that fixed enough to commit to the contrib issue.

πŸ› Exceptions in batch no longer are shown on the page when Javascript is disabled Needs work might still affect the build tests for Automatic Updates but once πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work is fixed we will apply the patch for this issue directly in the conversion script. Regardless though this issue should not affect the Package manager build test because don't use the batch form system in those. So until #3411110 we can just worry about getting the Package Manager build test passing.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

@phenaproxima I don't think I will have time to work on this soon, so I will leave assigned to you. We need to figure out why the test needs the working directory changed on 11.x

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I forgot that the patch we need only applies in 11.x and is only needed in 11.x . So we will just have to do it as part of πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I am working on πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work , it not committed but the problem should not affect all the build tests. Therefore we can start to work on this issue.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I fixed πŸ› Expand ConverterCommand documentation to make it easier to run. Fixed

You can the conversion locally this issues MR branch and then run the tests locally. You should be able to convert locally and then run the specific tests that fail to speed up fixing these issues

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Looks like there aren't that many tests that fail!

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I committed this because it only affects scripts/src/ConverterCommand.php

Marking this as fixed but if we need to work on the docs more we can open this issue again

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I have added a " Core Merge request planning 🌱 Drupal 10 Core Roadmap for Automatic Updates Active " section to the summary of 🌱 Drupal 10 Core Roadmap for Automatic Updates Active to list the issues that need to addressed

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Marking this as fixed

We should do this next πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work which require module changes to make it compatible with 11.x core
and then πŸ“Œ [PP-1] Fix build tests for Automatic Updates core Gitlab Postponed which a core scratch issue to get the build tests passing on the core merge requests

Getting the core build test running here is lesser priority then that other 2

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Postponing on πŸ“Œ Test converted on module on gitlabci on core 11.x Needs work which ensure that module is completely compatible with 11.x first. We can't currently test the module against core 11.x in drupalci and even if we could it would not be the converted core version

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Re #45, I added the links to core dependency and policy questions. It doesn't seem like 🌱 [policy, no patch] Consider whether to keep Package Manager and Automatic Updates in a separate repo/package than core in order to facilitate releasing updates to the updater Needs review but I added because it is still open and I don't think this something we would want to do later. So I think this should be closed if there consensus in core governance to do this

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Postponing on πŸ“Œ Run kernel tests on GitLab CI after module is converted to core version RTBC . Someone could work on this if they want but just marking to make it obvious we aren't working on it and don't need to right now

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

I want to try quick experiment in this area, though still leaving this as minor priority as πŸ“Œ Run kernel tests on GitLab CI after module is converted to core version RTBC is the primary concern for now

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Looks good. If tests passπŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

tedbow β†’ made their first commit to this issue’s fork.

Production build https://api.contrib.social 0.61.6-2-g546bc20