Merge pull request #35 from move-elevator/fix/feature-instance-deploy-path

fix: keep feature deployments off the base instance

Author: konradmichalik

deployer/dev/task/sync.php

@@ -11,7 +11,7 @@
         if (host('stage')->get('labels')['type'] === 'feature-branch-deployment') {
             on(host('stage'), function () {
                 $currentBranch = runLocally('git branch --show-current');
-                $target = !is_null(input()->getOption('feature')) ? input()->getOption('feature') : askChoice('Please select a sync origin', array_merge(
+                $target = featureRequested() ? input()->getOption('feature') : askChoice('Please select a sync origin', array_merge(
                     ["[current] ($currentBranch)", "[prod]"],
                     array_map(function ($array) {
                         return $array[2];

deployer/feature/task/deploy.php

@@ -6,7 +6,14 @@
  * Default extensions for deploy tasks
  */
 
-before('rollback', 'feature:init');
+/*
+ * deploy:info resolves {{release_name}} and deployer caches it for the rest of the
+ * run. Without this hook the counter is read from the base path and the cached name
+ * then collides in deploy:release, so the feature instance has to exist by now.
+ */
+before('deploy:info', 'feature:init');
+
+before('rollback', 'feature:select');
 before('deploy:unlock', 'feature:init');
 before('feature:sync', 'feature:init');
 before('deploy:setup', 'feature:setup');
@@ -16,7 +23,6 @@
 before('feature:sync', 'feature:wait_for_database');
 before('deploy:database:update', 'feature:wait_for_database');
 after('deploy:symlink', 'feature:urlshortener');
-before('feature:sync', 'feature:init');
-before('debug:db', 'feature:init');
-before('debug:ssh', 'feature:init');
-before('debug:log:app', 'feature:init');
+before('debug:db', 'feature:select');
+before('debug:ssh', 'feature:select');
+before('debug:log:app', 'feature:select');

deployer/feature/task/feature_init.php

@@ -8,13 +8,26 @@
 
 task('feature:init', function () {
     checkVerbosity();
+    if (!featureRequested()) {
+        debug('No feature given, staying on the base instance');
+        return;
+    }
     // extend deploy path / public url
     initFeature();
 })
     ->select('type=feature-branch-deployment')
     ->once()
     ->desc('Initialize a feature branch');
 
+task('feature:select', function () {
+    checkVerbosity();
+    // extend deploy path / public url, asking for the feature if none was given
+    initFeature();
+})
+    ->select('type=feature-branch-deployment')
+    ->once()
+    ->desc('Select a feature branch and initialize it');
+
 
 /**
  * Initialize a feature branch
@@ -36,9 +49,12 @@ function initFeature(?string $feature = null): ?string
 
     prepareDeployerConfiguration();
     // use feature variable or feature input option or ask for feature branch
-    $feature = $feature ?: (!is_null(input()->getOption('feature')) ? input()->getOption('feature') : askChoice('Please select a feature branch', array_map(function ($array) {
-        return $array[2];
-    }, listFeatureInstances())));
+    // (?: would discard a caller-provided "0", which is a valid instance name)
+    if (null === $feature || '' === trim($feature)) {
+        $feature = featureRequested() ? input()->getOption('feature') : askChoice('Please select a feature branch', array_map(function ($array) {
+            return $array[2];
+        }, listFeatureInstances()));
+    }
     set('feature', $feature);
 
     if (isUrlShortener()) {

deployer/feature/task/feature_notify.php

@@ -7,7 +7,7 @@
 
 task('feature:notify', function () {
 
-    if ((has('feature_setup') && !get('feature_setup')) || !input()->getOption('feature')) return;
+    if ((has('feature_setup') && !get('feature_setup')) || !featureRequested()) return;
     checkVerbosity();
 
     set('public_url', get('public_urls')[0]);

deployer/feature/task/feature_setup.php

@@ -8,7 +8,9 @@
 require_once('url_shortener.php');
 
 task('feature:setup', function () {
-    if (!input()->hasOption('feature')) {
+    // Without a feature we are deploying the base instance and must not touch any
+    // feature scaffolding — renderRemoteTemplates() would overwrite its .env.
+    if (!featureRequested()) {
         return;
     }
     checkVerbosity();

deployer/feature/task/feature_sync.php

@@ -102,7 +102,7 @@ function resolveDatabaseHostToIp(string $hostname): void
 
 
 task('feature:wait_for_database', function () {
-    if ((has('feature_setup') && !get('feature_setup')) || !input()->getOption('feature')) return;
+    if ((has('feature_setup') && !get('feature_setup')) || !featureRequested()) return;
     waitForDatabaseHost();
 })
     ->select('type=feature-branch-deployment')
@@ -113,7 +113,7 @@ function resolveDatabaseHostToIp(string $hostname): void
 
 task('feature:sync', function () {
 
-    if ((has('feature_setup') && !get('feature_setup')) || !input()->getOption('feature')) return;
+    if ((has('feature_setup') && !get('feature_setup')) || !featureRequested()) return;
 
     $feature = initFeature();
     $synced = false;

deployer/feature/task/url_shortener.php

@@ -2,9 +2,9 @@
 
 namespace Deployer;
 
-tasK('feature:urlshortener', function () {
+task('feature:urlshortener', function () {
 
-    if (!input()->getOption('feature')) {
+    if (!featureRequested()) {
         return;
     }
     if (!isUrlShortener()) {

deployer/functions.php

@@ -34,6 +34,27 @@ function checkVerbosity(): void
     }
 }
 
+/**
+ * Whether a feature instance was addressed via --feature.
+ *
+ * A blank --feature= counts as absent: it would otherwise resolve to the base
+ * instance path and let the feature scaffolding write into the reference stage.
+ * Only null and blank strings qualify — empty() would also swallow the perfectly
+ * valid feature name "0". The option itself only exists once the feature recipe
+ * is loaded, hence hasOption().
+ *
+ * @return bool
+ */
+function featureRequested(): bool
+{
+    if (!input()->hasOption('feature')) {
+        return false;
+    }
+    $feature = input()->getOption('feature');
+
+    return null !== $feature && '' !== trim((string)$feature);
+}
+
 /**
  * Extend the deployer configuration with available environment variables (starting with "DEPLOYER_CONFIG_"):
  *

docs/FEATURE.md

@@ -55,11 +55,9 @@ The `feature:setup` command represent the initialization of a new feature branch
 $ vendor/bin/dep feature:setup stage --feature=TEST-01
 ```
 
-This task should be declared to run at first within your deploy task:
+The recipe already wires this task into the deploy flow, together with a `feature:init` before `deploy:info`. That ordering matters: `deploy:info` resolves `{{release_name}}` and deployer caches the result for the rest of the run, so the feature instance has to be known before it runs. Do not hook `feature:setup` any earlier yourself.
 
-```php
-before('deploy:info', 'feature:setup');
-```
+> Upgrading: if your `deploy.php` carries a `before('deploy:info', 'feature:init')` (or an equivalent `feature:setup` hook) as a workaround for that ordering, remove it — the recipe registers it now and the hook would otherwise run twice.
 
 If the application needs to setup additional configuration files for e.g. storing the database credentials, use the feature templates to provide this kind of dynamic setup. For example the TYPO3 setup with a `.env` file:
 
@@ -86,8 +84,16 @@ You can extend these list be providing more environment variables starting with
 > Hint: If you're using other deployer commands within the feature branch deployment context, you should use the `feature:init` task to extend the host definition with the necessary feature instance configuration:
 >
 > ```php
-> before('deploy:rollback', 'feature:init');
+> before('my:task', 'feature:init');
 > ```
+>
+> `feature:init` is a no-op without `--feature`, so the command keeps operating on the base instance. Use `feature:select` instead if the command should offer an interactive choice between the existing feature instances when `--feature` is omitted:
+>
+> ```php
+> before('my:task', 'feature:select');
+> ```
+>
+> `rollback` and the `debug:*` tasks are already wired to `feature:select` by the recipe.
 
 ### Deletion