#263: Refactor Tugboat integration #1073
Conversation
rabbitlair
requested review from
YesCT,
beto-aveiga,
deviantintegral and
mrdavidburns
Summary of ChangesHello @rabbitlair, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Tugboat integration within the project. The primary goal is to centralize configuration management by using DDEV settings as the authoritative source for all service definitions. This is achieved through a new dedicated Composer plugin that dynamically generates the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great refactoring of the Tugboat integration. Moving the logic to a dedicated plugin and adopting a template-based approach significantly improves maintainability and extensibility. The new TugboatConfigPlugin is well-structured. However, I've identified several critical regressions where conditional logic from the old shell scripts was lost in the new Twig templates, potentially breaking functionality for some project configurations (e.g., non-MariaDB databases, Memcached, Apache, Pantheon). I've also included some suggestions to improve code clarity and fix a bug in the new plugin. After addressing these points, this will be a very solid contribution.
| - | | ||
| # Install mysql or mariadb client. | ||
| apt-get update | ||
| apt-get install -y mariadb-client | ||
| - | | ||
| # Link the document root to the expected path. Tugboat uses /docroot | ||
| # by default. So, if Drupal is located at any other path in your git | ||
| # repository, change that here. This example links /web to the docroot | ||
| ln -snf "${TUGBOAT_ROOT}/web" "${DOCROOT}" | ||
| - | | ||
| # Install the PHP opcache as it's not included by default and needed for | ||
| # decent performance. | ||
| docker-php-ext-install opcache | ||
| - | | ||
| # GD dependencies. | ||
| apt-get install -y libpng-dev libjpeg-dev libfreetype6-dev | ||
| - | | ||
| # WebP dependencies. | ||
| apt-get install -y libwebp-dev libwebp7 webp libmagickwand-dev | ||
| - | | ||
| # Build and install gd. | ||
| docker-php-ext-configure gd --with-freetype --with-jpeg --with-webp | ||
| docker-php-ext-install gd | ||
| - | | ||
| # Install ImageMagick. This is recommended by both Acquia and Pantheon instead | ||
| # of GD. Lullabot will likely be publishing an ADR recommending it too. | ||
| apt-get install -y imagemagick | ||
| - | | ||
| # Install the PHP redis extension. | ||
| yes '' | pecl install -f redis | ||
| echo 'extension=redis.so' > /usr/local/etc/php/conf.d/redis.ini | ||
| - | | ||
| # Create the Drupal private and public files directories if they aren't | ||
| # already present. | ||
| mkdir -p "${DOCROOT}/sites/default/files" | ||
| chmod 777 "${DOCROOT}/sites/default/files" | ||
| chgrp -R www-data "${DOCROOT}/sites/default/files" | ||
| - composer install | ||
| - | | ||
| # Install node | ||
| apt-get install -y ca-certificates gnupg | ||
| mkdir -p /etc/apt/keyrings | ||
| curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | sudo gpg --dearmor -o /etc/apt/keyrings/nodesource.gpg | ||
| NODE_MAJOR=$(cat ${TUGBOAT_ROOT}/.nvmrc) | ||
| echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_$NODE_MAJOR.x nodistro main" | sudo tee /etc/apt/sources.list.d/nodesource.list | ||
| - | | ||
| # Ensure we use nodesource packages, even if Debian ships newer major versions. | ||
| echo 'Package: *' > /etc/apt/preferences.d/nodesource-nodejs | ||
| echo 'Pin: origin deb.nodesource.com' >> /etc/apt/preferences.d/nodesource-nodejs | ||
| echo 'Pin-Priority: 1001' >> /etc/apt/preferences.d/nodesource-nodejs | ||
| - apt-get update | ||
| - apt-get -qq install nodejs | ||
| - apt-get clean | ||
| - | | ||
| # This only works for node > 16, but that version is unsupported now anyway. | ||
| corepack enable | ||
| - | | ||
| # Validate we have the right nodejs version. | ||
| nodejs -v | grep -q v$NODE_MAJOR | ||
| - | | ||
| # Install task | ||
| TASKFILE=$(cat ${TUGBOAT_ROOT}/vendor/lullabot/drainpipe/.taskfile) | ||
| sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d ${TASKFILE} -b /usr/local/bin | ||
| {% if services.php.commands.init|default(false) %} | ||
| - task tugboat:php:init | ||
| {% endif %} |
There was a problem hiding this comment.
This template has several regressions compared to the previous implementation, which can break builds for various project configurations. Specifically:
- Database Client: It hardcodes
mariadb-clientinstallation. This will fail for projects using PostgreSQL or MySQL, as thephpcontainer won't have the correct database client. - PHP Extensions: It unconditionally installs the
redisextension and completely omits thememcachedextension. This should be conditional based on the detectedmemory_cacheservice. - Apache Support: It's missing the
a2enmod headers rewritecommand for Apache webservers. - Pantheon Support: It's missing the installation of the Pantheon Terminus CLI for projects with Pantheon integration enabled.
I've provided a complete replacement for the file that restores the correct conditional logic for these features.
- |
# Install database client.
apt-get update
{% if services.database.type == 'mysql' %}
# mysql was replaced by mariadb in Debian, so we have to install it manually.
# https://dev.mysql.com/doc/mysql-apt-repo-quick-guide/en/#repo-qg-apt-repo-manual-setup
# apt-key is also deprecated, so here we install the key manually.
./.tugboat/scripts/install-mysql-client.sh
{% else %}
apt-get install -y {{ services.database.type }}-client
{% endif %}
- |
# Link the document root to the expected path. Tugboat uses /docroot
# by default. So, if Drupal is located at any other path in your git
# repository, change that here. This example links /web to the docroot
ln -snf "${TUGBOAT_ROOT}/web" "${DOCROOT}"
- |
# Install the PHP opcache as it's not included by default and needed for
# decent performance.
docker-php-ext-install opcache
- |
# GD dependencies.
apt-get install -y libpng-dev libjpeg-dev libfreetype6-dev
- |
# WebP dependencies.
apt-get install -y libwebp-dev libwebp7 webp libmagickwand-dev
- |
# Build and install gd.
docker-php-ext-configure gd --with-freetype --with-jpeg --with-webp
docker-php-ext-install gd
- |
# Install ImageMagick. This is recommended by both Acquia and Pantheon instead
# of GD. Lullabot will likely be publishing an ADR recommending it too.
apt-get install -y imagemagick
{% if services.memory_cache.type == "memcached" %}
- |
# Install the PHP memcache extension.
apt-get install -y zlib1g-dev
yes '' | pecl install -f memcache
echo 'extension=memcache.so' > /usr/local/etc/php/conf.d/memcache.ini
{% elseif services.memory_cache.type == "redis" %}
- |
# Install the PHP redis extension.
yes '' | pecl install -f redis
echo 'extension=redis.so' > /usr/local/etc/php/conf.d/redis.ini
{% endif %}
- |
# Create the Drupal private and public files directories if they aren't
# already present.
mkdir -p "${DOCROOT}/sites/default/files"
chmod 777 "${DOCROOT}/sites/default/files"
chgrp -R www-data "${DOCROOT}/sites/default/files"
- composer install
- |
# Install node
apt-get install -y ca-certificates gnupg
mkdir -p /etc/apt/keyrings
curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | sudo gpg --dearmor -o /etc/apt/keyrings/nodesource.gpg
NODE_MAJOR=$(cat ${TUGBOAT_ROOT}/.nvmrc)
echo "deb [signed-by=/etc/apt/keyrings/nodesource.gpg] https://deb.nodesource.com/node_$NODE_MAJOR.x nodistro main" | sudo tee /etc/apt/sources.list.d/nodesource.list
- |
# Ensure we use nodesource packages, even if Debian ships newer major versions.
echo 'Package: *' > /etc/apt/preferences.d/nodesource-nodejs
echo 'Pin: origin deb.nodesource.com' >> /etc/apt/preferences.d/nodesource-nodejs
echo 'Pin-Priority: 1001' >> /etc/apt/preferences.d/nodesource-nodejs
- apt-get update
- apt-get -qq install nodejs
- apt-get clean
- |
# This only works for node > 16, but that version is unsupported now anyway.
corepack enable
- |
# Validate we have the right nodejs version.
nodejs -v | grep -q v$NODE_MAJOR
- |
# Install task
TASKFILE=$(cat ${TUGBOAT_ROOT}/vendor/lullabot/drainpipe/.taskfile)
sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d ${TASKFILE} -b /usr/local/bin
{% if services.php.type == 'apache-fpm' %}
- |
# Apache
a2enmod headers rewrite
{% endif %}
{% if pantheon %}
- |
# Pantheon
curl --fail -sSL https://github.com/pantheon-systems/terminus/releases/download/$(curl -L --fail --silent "https://api.github.com/repos/pantheon-systems/terminus/releases/latest" | perl -nle'print $& while m{"tag_name": "\K.*?(?=")"}g')/terminus.phar --output /usr/local/bin/terminus
chmod 777 /usr/local/bin/terminus
{% endif %}
{% if services.php.commands.init|default(false) %}
- task tugboat:php:init
{% endif %}
| # Install YQ | ||
| wget https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/bin/yq && chmod +x /usr/bin/yq |
There was a problem hiding this comment.
Installing yq from the latest release can introduce unexpected breaking changes into the build process, which can lead to CI failures. It's a best practice to pin dependencies to specific versions to ensure reproducible builds, similar to how the task binary version is handled in this script.
| # Install YQ | |
| wget https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 -O /usr/bin/yq && chmod +x /usr/bin/yq | |
| # Install YQ | |
| YQ_VERSION=v4.40.5 # Pin to a specific version to ensure build stability. | |
| wget https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64 -O /usr/bin/yq && chmod +x /usr/bin/yq |
| $vendor = $this->config->get('vendor-dir'); | ||
| $scaffoldPath = $vendor . '/lullabot/drainpipe/scaffold/tugboat/templates'; | ||
| $loader = new FilesystemLoader($scaffoldPath); | ||
| $twig = new Environment($loader); |
There was a problem hiding this comment.
The generateSettingsFile method initializes its own Twig environment, which does not respect project-level template overrides in .tugboat/drainpipe-templates/. If a user overrides settings.tugboat.php.twig, their changes will be ignored. This can be fixed by using the initTwigLoader() helper method, which correctly sets up the template paths.
$twig = $this->initTwigLoader();| $commands['php']['update'] = 'build:drupal'; | ||
| if (isset($tasks['update'])) { | ||
| $commands['php']['update'] = 'update'; | ||
| } | ||
| if (isset($tasks['update:tugboat'])) { | ||
| $commands['php']['update'] = 'update:tugboat'; | ||
| } |
There was a problem hiding this comment.
The current logic to determine the update command for the php service is a bit hard to follow due to setting a default and then conditionally overriding it. Using a more standard if/elseif/else structure would make the precedence clearer and improve maintainability.
if (isset($tasks['update:tugboat'])) {
$commands['php']['update'] = 'update:tugboat';
}
elseif (isset($tasks['update'])) {
$commands['php']['update'] = 'update';
}
else {
$commands['php']['update'] = 'build:drupal';
}
src/TugboatConfigPlugin.php
Outdated
| return $services; | ||
| } | ||
|
|
||
| private function initTwigLoader(Environment &$twig = null): Environment |
There was a problem hiding this comment.
The method signature private function initTwigLoader(Environment &$twig = null): Environment is a bit confusing. The pass-by-reference parameter $twig is not used as such, and the function always creates and returns a new Environment instance. A simpler signature would improve code clarity.
private function initTwigLoader(): Environment
|
|
||
| # Install Taskfile | ||
| TASKFILE=$(cat ${TUGBOAT_ROOT}/vendor/lullabot/drainpipe/.taskfile) | ||
| sh -c "$(curl --location https://taskfile.dev/install.sh)" -- -d ${TASKFILE} -b /usr/local/bin |
There was a problem hiding this comment.
Let's pin this to a version, and add a Renovate custom manager to keep it updated?
There was a problem hiding this comment.
The Taskfile version is already pinned by the .taskfile file :)
| # environment: | ||
| services: | ||
| php: | ||
| http: false |
There was a problem hiding this comment.
huh, should this be true?
There was a problem hiding this comment.
No, setting http to false tells the tugboat container to not listen to plain HTTP traffic, andforces all traffic to go via https instead
| - `php-init.yml.twig` - Init phase commands | ||
| - `php-update.yml.twig` - Update phase commands | ||
| - `php-build.yml.twig` - Build phase commands | ||
| - `config.yml.twig` - Complete Tugboat configuration (advanced) |
|
This one is now ready for review. Tests have been fixed, and the Drainpipe site in Tugboat is able to build properly. Please, any feedback will be more than welcome. Thanks! |
|
@rabbitlair perhaps as a first step, either fix or resolve the Gemini comments? Do you have test steps you'd like people to go through? |
Closes #263
This PR refactors the Tugboat integration to use DDEV configuration as the single source of truth for all service definitions. The new implementation generates a complete
config.ymlusing Twig templates instead of separate bash scripts.Architecture
TugboatConfigPluginhandles all Tugboat logic (previously inScaffoldInstallerPlugin)config.ymlfile with sub-templates for each build phase in main PHP service.ddev/config.yamland docker-compose add-on filesBuild Phase Templates
php-init.yml.twig- System packages, PHP extensions, Node.js installation, composer installphp-update.yml.twig- Composer install, sync task, file permissionsphp-build.yml.twig- Build and update tasksconfig.yml.twig- Main configuration template that includes phase templatesTaskfile Integration
sync:tugboat,update:tugboat,build:tugboat,online:tugboattaskstugboat:{service}:inittasks (e.g.,tugboat:redis:init)custom-init-command.shwrapper for non-PHP service init tasks.taskfilefileTemplate Customization
vendor/lullabot/drainpipe/scaffold/tugboat/templates/.tugboat/drainpipe-templates/indentfilter for proper YAML formattingFiles Structure
Please, note this is a first working version. I would like to know your thoughts about the new approach - any feedback will be more than welcome. As far as I can tell, this refactor does not break backwards compatibility while adding the custom templates feature, which I think will make easier for users to customize their Tugboat integration. Also, take into account tests are broken - once we all are happy with the approach and implementation, I will fix those tests failing.