laravel / pennant

A simple, lightweight library for managing feature flags.
https://laravel.com/docs/pennant
MIT License
479 stars 49 forks source link

[1.x] Add ability to prune undefined features #87

Closed jameshulse closed 8 months ago

jameshulse commented 8 months ago

Overview

This PR adds the ability to remove old unused features from your data store. When a feature flag definition has been removed, rather than having to manually purge the now undefined feature we can instead run an automated prune.

The logic in this PR is to look at the list of 'defined' features and remove any other features.

This will allow us to add the new php artisan pennant:prune command to our deployments and it will automatically clean up feature flags from the database that we no longer have defined.

Benefits

Having this directly in Pennant saves us (and others) from implementing similar functionality in their own applications.

The changes are self-contained so shouldn't affect any existing functionality.

timacdonald commented 8 months ago

@jameshulse, Pennant does not require you to "register" features to use them. They can be dynamically registered, for example, if I have the following feature...

<?php

namespace App\Features;

use Illuminate\Support\Lottery;

class NewApi
{
    public function resolve(User $user): mixed
    {
        return Lottery::odds(1 / 100);
    }
}

it is possible to use the feature without registering it in a service provider. In a blade file I could do the following...

@if (Feature::active(NewApi::class))
    Using API v2
@endif

This means that there is no way to know that you only purging features no longer in use.

I think this feature is useful, but perhaps we should use the existing pennant:clear / pennant:purge command and add an --except flag so that feature flags may be explicitly kept on a case-by-case basis.

php artisan pennant:clear --except=foo,bar,baz

We could then also add a DX improvement for your wants that determines what not to clear automatically based on what is registerd.

php artisan pennant:clear --except-registered

This would then do what your new command is doing under the hood.

This means:

  1. Those using dynamic registration could use the --except flag.
  2. Those using explicit registration or auto-discovery could use the --except-registered flag.

Thoughts?

jameshulse commented 8 months ago

Thanks @timacdonald, that's a good catch. I had a feeling that my solution could be closely tied to how we are using the library, so it's great to get that insight.

Your solution seems like a good one.

I will have a go at implementing php artisan pennant:clear --except-registered shortly.

jameshulse commented 8 months ago

I'm running out of time to look at this today. Regarding solution direction: I'm thinking of adding an $exclude parameter to the existing purge method:

public function purge($features = null, $except = null): void

Then the pennant:clear command can determine what set of features to pass to the except parameter:

  1. Pass any features from the --exclude=* option
  2. If the --except-registered option is passed, then get all defined features and also pass them.

Roughly:

public function handle(FeatureManager $manager)
{
    $store = $manager->store($this->option('store'));

    $store->purge(
        features: $this->argument('features') ?: null,
        except: array_unique(array_merge($this->option('except') ?? [], $this->option('except-registered') ? $store->defined() : []))
    );

    with($this->argument('features') ?: ['All features'], function ($names) {
       $this->components->info(implode(', ', $names).' successfully purged from storage.');
    });

    return self::SUCCESS;
}

Then the decorator and drivers can handle excluding those 'excepted' feature names.

timacdonald commented 8 months ago

Unfortunately we can't change the purge method signature, as it will be a breaking change.

If you don't have time to dive in I'm happy to take the lead. If you wanna keep digging into it, we will likely need to do the lifting for the drivers in the command.

Something like this (a simplified version)...

$all = $store->defined();

$features = Arr::except($all, $this->argument('features'));

$store->purge($features);

So I would imagine the only changes would be in the artisan command class.

jameshulse commented 8 months ago

Unfortunately we can't change the purge method signature, as it will be a breaking change.

Yikes. I'm glad you're all over this stuff.

If you could take the lead from here it'd be much appreciated!

timacdonald commented 8 months ago

I've re-worked this idea over here: https://github.com/laravel/pennant/pull/89