Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We currently exclude site-specific packages from hosting_package_instance. This can lead to some odd behaviour, such as a module appearing on a platform package list, but not on the package list of a site on that platform.
Implementing this would allow us to better manage sites, such as being able to enable, disable download and update modules on sites.
Comments
Comment #1
omega8cc CreditAttribution: omega8cc commentedNot sure if I understand the intention here, but site specific packages have been excluded by design - because then you will easily break ability to upgrade. It is a discouraged practice to use site specific space for any code, because it creates an unavoidable chicken/egg syndrome on upgrade, which essentially depends on differences between codebases (platforms). When you are using site specific code space you are making upgrades impossible, because there are no different codebases to compare and check for upgrades/compatibility etc. See also: http://community.aegirproject.org/node/243
Comment #2
ergonlogicSite-specific modules and themes are explicitly excluded in provision_drupal_system_map(). If we remove these bits, we'll need a way to flag them as site-specific in the hosting_package_instance table, so that they are ignored during platform comparisons.
While we could add a column just to keep track of this, I'd prefer that the column hold the platform nid, or -1 for site-specific packages. This would provide us the necessary flag, and possibly simplify some of our Views. In particular, I think it might make #2035873: Show package 'popularity' on platform package view easier to implement.
Comment #3
ergonlogicThis turned out to be pretty simple. I've made a couple commits in 'dev/site-packages' in both Provision and Hosting that implements the basics. We exclude site-specific packages from platform comparisons, so that we don't get false-positives.
Comment #4
ergonlogicOops, I missed the comment in #1. Basically, the point is to allow things like updating the modules in a site too, among other things. We basically have #1976922: Add an site 'update' task now, which takes a backup before running updates, and rolls back by restoring from the backup. We could add an 'upgrade' task that hooks into an 'update' task and runs something like
drush up --no-core
. This should give us a safe upgrade process for site-specific packages.Comment #5
ergonlogicI believe that #604800: platform comparison doesn't handle properly non-global modules is the original issue where we decided to ignore site-specific modules. There wasn't much discussion there, and I'd like to re-open the debate. However, I did discover that as it stands, we'd suffer from the symptoms described in that issue.
Comment #6
ergonlogicActually, the inconsistency was fixed by running 'verify' on the site. I had cloned the site so I guess there isn't a verify triggered following the 'import' task? If so, then we should probably just make sure to verify there.
Comment #7
omega8cc CreditAttribution: omega8cc commentedIt is an old, known issue discussed many times, and the reason why we have added an auto-run Verify task to Migrate task in BOA. There is a built-in verify, but it doesn't work -- you need to run verify via frontend to make it work.
Comment #8
omega8cc CreditAttribution: omega8cc commentedBut in the principle, I would vote against encouraging people to use site specific space for code, since it is against Aegir core concepts.
Comment #9
omega8cc CreditAttribution: omega8cc commentedSee for reference: #1004526: Automatic aliases are not persisted across rename and clone
Comment #10
ergonlogicActually, the problem was two-fold. First, when verifying a platform, we sync package instances for profiles, which I hadn't accounted for. Secondly, I wasn't properly saving the 'platform' field when creating package instances, hence why the update that happened on the next verify fixed it. So an additional 'verify' task in not needed here.
Comment #11
helmo CreditAttribution: helmo commentedI didn't have time to properly test this yet, but committed one bugfix to the dev/packages-count branch.
Comment #12
anarcat CreditAttribution: anarcat commentedI don't like the idea of *requiring* a verify before we migrate sites.
The reason why those modules were ignored is exactly that: we didn't want to have to run verify on all sites (which includes clearing cache and all sorts of performance impacts) before migrating.
However, this was necessary, I believe, because we were considering site-specific packages in the site platform comparisons.
Now on with a code review:
watchdog documentation says the first argument is "The category to which this message belongs. Can be any string, but the general practice is to use the name of the module calling watchdog()."
So '[HOSTING PACKAGE]' should be hosting_package.
Also, there's a todo here:
why is that: does the patch work without those verify? why do we need to clean that table? See also #1034520: Cleanup zombie entries in hosting_package_* tables?.
Finally, I feel it is a bit late in the release cycle to try to introduce such a change. If you are confident it works, go ahead, but I really don't want to start messing around with failing migrate paths in the frontend in 2.x again. Remember that this is your last chance to change the API (so the database structure!) - if we mess it up in the 2.0 release, it's hard to go back if the DB is not sound...
But I will trust your judgement on this, you see that have gotten a good understanding of how this code works.
Comment #13
anarcat CreditAttribution: anarcat commentedIn fact, this is needs work - maybe just cosmetic in case of that watchdog() call, or an actual "needs implementation" for that todo.
Comment #14
ergonlogicActually, the todo isn't needed anymore.
Comment #15
ergonlogicfixed in fef1c13. I'd like to merge this, but it was forked off of the branch where I was working on #2031747: Properly control Views access, so I'll wait to see where we stand with that issue first.
Comment #16
ergonlogicMerged into hosting and provision 6.x-2.x.