Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin system for Diff* engine #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ambsw-technology
Copy link

This PR builds on refactor, adding a plugin system for Diff* engines. To make the plugin system work, I needed to simplify the interface between ParameterStore (formerly RemoteState) and DiffResolver (formerly FlatDictDiffer). My proposed interface is:

  • DiffBase.configure(args) is a standard way for plugins to pre-configure themselves using CLI args. This method would normally use functools.partial to pre-populate constructor kwargs.
    • (backwards incompatible) I also tweaked the CLI arg for --force (to --diffresolver-force) since it's specific to the DiffResolver plugin.
  • DiffBase.__init__(remote, local, **kwargs) initializes the plugin with the local and remote data. The constructor kwargs can be populated directly by programmatic users.
  • <instance>.plan (an @property) returns a dict of operations that will be applied to Parameter Store:
    {
        'add': {'key': '<value>', ...},
        'change': {
            'key': {'old': '<value>', 'new': '<value>'},
            ...
        },
        'delete': {'key': '<value>', ...}
    }
    
  • DiffBase.describe_diff(plan) will provide a print-friendly display of the elements in plan. Since the possible operations in plan are finite and well-described (i.e. what you can do on the AWS API), the default implementation should work for 99% of implementations.

By consolidating all API operations in a plan dict, the Diff* plugin can support an arbitrary amount of complexity. For example, a concurrency-sensitive plugin (like the one proposed in #16) could use additional methods to change the behavior of plans:

> diff = ImaginaryDiff(remote, local)
> diff.plan
Exception:  Conflicts Detected:
`/a/b/c/` has changed on both the remote and local storage
> diff.resolve('/a/b/c', use=ImaginaryDiff.REMOTE)
> diff.plan
{'add': {}, 'change': {}, 'delete': {}}
> diff.resolve('/a/b/c', use=ImaginaryDiff.LOCAL)
> diff.plan
{'add': {}, 'change': {'/a/b/c': {'old': '<remote>', 'new': '<local>'}}, 'delete': {}}

ImaginaryDiff would need local_changes() and remote_changes() instead of simply changed(). The result of these methods would be unchanged by resolve() but the plan would be updated accordingly.

I'm not married to the API if you can think of a situation that isn't addressed by it. I needed to get something working and this has been adequate so-far.

…, and better encapsulate behaviors (among other things to simplify testing)
…lified/standardized the interface (i.e. `configure`, `merge`, `plan`, and `__init__`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants