Add concept of build properties to Robo

grasmash commented 7 years ago

From #509:

Most task runners incorporate the concept of a "build properties" file, which allows a set of key value pairs to be defined in a file and passed to the task runner at run time. One particularly useful feature of Ant and Phing is that they permit property expansion in such files, see build.yml for example. Additionally, there is a convention for overriding such properties via CLI arguments. E.g., robo -Dkey=value. Is there any precedent for using such a file with Robo?

I've created a standalone tool that at least handles the property expansion feature: https://github.com/grasmash/yaml-expander

Requirements:

[x] Look for YAML config file in default location and load prior to command execution. E.g., app/config.yml. Config should be parsed, expanded, and made available to commands through a service.
[x] Add ability to define runtime values for build properties. E.g., robo my:command --key=value. This would override values loaded from config file.

greg-1-anderson commented 7 years ago

+1 on this idea

greg-1-anderson commented 7 years ago

Robo already contains a simple Config class. Terminus expands on this to provide more capabilities.

It would also be good to be able to define configuration values that are injected into: a) command options b) task setter methods

DavertMik commented 7 years ago

Looks good, except that .env sholdn't be parsed by parse_ini_string :) There are some edge cases

How to can this config providers be used with Robofile? Or you need to extend a Runner for that?

greg-1-anderson commented 7 years ago

My idea for RoboFiles is that the config should have some specific section for commands. For example:

command:
  foo:
    options:
      bar: baz

Then, in your RoboFile:

public function foo($options = ['bar' => ''])
{
    $this->io()->writeln("The option is {$options['bar']}");
}

The result of ./robo foo would then be: The option is baz.

Similar things could be done with tasks, if you wanted to configure every instance of some task's setter (not explicitly overridden by a command) to some default value.

greg-1-anderson commented 7 years ago

cc: @weitzman

weitzman commented 7 years ago

This looks like a great direction IMO. Drush config would likely switch to the system proposed here.

daften commented 7 years ago

Just FYI, we're working on this problem too. The main thing we're struggling with is property overrides through the command-line. Our current solution allows to read properties files throughout the project, see https://github.com/digipolisgent/robo-digipolis-general/blob/develop/src/Common/DigipolisPropertiesAware.php

The CLI overrides are hindered by the fact you can't define an option CLI that is not in the command, robo throws an error.

CC @Jelle-S

greg-1-anderson commented 7 years ago

@daften I believe that if you use an appropriate @hook, you can alter the input object after validation is done. Either @hook init or @hook validate should work. Hook validate says:

"this is better done in the configure hook"

I believe this is supposed to say "better done in the initialize hook".

Initialize looks up options from configuration and applies them to $input iff there is not already a value there.
Interact will prompt the user for a value for options that have not already been provided by the user or configuration.
Validate confirms that the values provided are appropriate for the current command.

I will likely be working on the first step (applying configuration in @hook init) shortly.

jalogut commented 7 years ago

Hello everyone,

I am considering moving from PHing to Robo but I am also missing the properties functionality. If no one is at the moment working on that, I would love to take over and implement an Extension for it. My solution would be pretty similar to how PHing properties work.

I could implement it as follows:

Classes & Traits

class TaskLoadProperties
- load from $args
- load from file
trait PropertiesAwareTrait
- manage properties globally.
- setter
- getter

Command example

Having this 2 new elements, we could use them as follows from any task or command

<?php
    use PropertiesAwareTrait;

    function command(array $args)
    {
       $this->taskLoadProperties()
           ->args($args)
           ->file($absolutePath)
           ->file($absolutePath)
           ->run();

       $test1 = $this->getProperty('test1');
       $this->setProperty('test3', 'value3');
    }
?>

For passing $args in a command, I would use pass-trough arguments functionality using -- syntax:

robo.phar command -- property.one=value1 property.two=value2 anyKey=value3

Properties load Priority

$args from command line will have highest priority and will overwrite properties loaded from any file.
Properties loaded from files will have a decremental priority. If 2 files have same property key, the first loaded will prevail.

PropertiesAwareTrait

PropertiesAwareTrait will look like something that:

use Robo\Robo;

trait PropertiesAwareTrait
{
    public function setProperty($key, $value)
    {
        $properties = $this->getProperties();
        $properties[$key] = $value;
        Robo::config()->set('properties', $properties);

        return $this;
    }

    public function getProperty($key)
    {
        return $this->getProperties()[$key];
    }

    public function getProperties()
    {
        $properties = Robo::config()->get('properties');
        if (!$properties) {
            $properties = [];
        }
        return $properties;
    }
}

Known issues

With this solution would not be possible to use optional params together with $args as explained on https://github.com/consolidation/Robo/issues/551

daften commented 7 years ago

@jalogut Please take a look at https://github.com/digipolisgent/robo-digipolis-general If nothing else, it can serve as inspiration.

The main problem we haven't figured out yet, is reading command-specific properties from the command-line (like phing does with -D options).

cc @Jelle-S

greg-1-anderson commented 7 years ago

@jalogut Thank you for your interest in working on this issue.

First, I want to clarify your requirements. I am wondering if there is any particular reason why you express your command properties as follows:

robo.phar foo -- property.one=value1 property.two=value2 anyKey=value3

Is there any reason this couldn't simply be:

robo.phar foo --property-one=value1 property-two=value2 --anyKey=value3

Then, you would set these in any config file via:

command:
  foo:
    options:
      property-one: baz
      property-two: boz
      anykey: xyzzy

That is the current design. Will that work for you?

Also, regarding reading properties in a command, I've seen a couple of suggestions for a PropertiesAwareTrait or a taskReadProperties. I don't see the need for either of these. I was thinking of something a little simpler and a little more flexible.

Given:

$this->taskComposerInstall()
  ->dir($buildDir)
  ->printed(false)
  ->run();

You could, then, instead do this:

$this->taskComposerInstall()
  ->run();

And use the following configuration:

task:
  ComposerInstall:
    dir: /my/path
    printed: false

This would call ->dir("/my/path") and ->printed(false) for any taskCompoerInstall() used anywhere.

To limit this to taskComposerInstall() in one particular command, then move the task block under the specific command:

command:
  foo:
    options:
      property-one: baz
      property-too: boz
      anykey: xyzzy
    task:
      ComposerInstall:
        dir: /my/path
        printed: false

While this is fine, folks may want to define better configuration for their commands. If we define a remap from command.foo.task.ComposerInstall.dir: command.foo.install-dir (mechanism(s) tbd), then:

command:
  foo:
    install-dir: /my/path
    options:
      property-one: baz

Should you happen to have a command that uses the same task multiple times, and needs to use different values for the setters, or if you have a value that needs to be provided to multiple task setter methods, then you go back to using code with options:

$this->taskComposerInstall()
  ->dir($options['install-dir'])
  ->printed($options['printed'])
  ->run();

And, of course, we can also remap options: command.foo.options.install-dir: command.foo.install-dir.

By these mechanisms, commands should rarely need to reference the Config object directly. If they do, though, I still don't think there is any need to introduce anything new such as PropertiesAwareTrait or taskReadProperties, as it is already possible to declare command classes to implement ConfigAwareInterface and use ConfigAwareTrait, at which point $this->config() becomes available.

In addition to remapping configuration properties, we should also allow configuration files to contain replacements, e.g.:

constants:
  BASE_DIR: /my/path
  COMMON_VALUE: baz
command:
  foo:
    install-dir: ${BASE_DIR}/subdir
    options:
      property-one: ${COMMON_VALUE}

I'm not at all set on or attached to the example syntax above; there are a lot of things that could work here that would be equivalent. Proposals welcome, especially proposals involving existing config systems that we should emulate for better standardization. Using Symfony Config for some things has been considered; I'm not sure if it offers this specific feature.

It would be good to know if something like the above would meet everyone's needs, or, if not, what the functionality gaps are.

@nerdstein is working on some of this right now (on the options side, anyway). I've been meaning to carve out some time to assist all week; perhaps I'll be able to put some code together tomorrow. If anyone wants to take on some of the functions that need to be written, help would be appreciated.

grasmash commented 7 years ago

A few thoughts:

I think we should start with the ability for Robo to read a config file with arbitrary configuration. The effort of then taking that configuration and mapping it to tasks seems like a follow-on.
For property expansion of yaml properties, I've created grasmash/yaml-expander.
I've already implemented a personal version of the configuration parser. See YamlConfig, which allows dot-notated yaml files to be used and incorporates property expansion. Example usage here

jalogut commented 7 years ago

@Hi @greg-1-anderson,

Thank you very much for your detailed answer. I think this would also satisfy our needs. It seems to be a more customisable an extensible solution. It looks very promising!

A couple of questions/suggestions:

How to define which configuration files to load?
Would it be possible to load several configuration files and that the latest overwrite values from previous ones? Imagine that I create an application with robo with default settings in a configuration file, folks should be able to overwrite some settings on his own configuration file.
Would also be possible to pass an array parameter to a task as follows?

Task:
    ComposerInstall:
         userDetails:
              Name: Juan
              Lastname: Alonso
              Emal: some@domain.com
              //...

One stupid question just to be sure that I am not missing anything. None of these features that you mentioned are available yet, right?

greg-1-anderson commented 7 years ago

@grasmash Thanks for reminding me about grasmash/yaml-expander. Momentarily forgot about that -- we should use it here. Also, it looks like YamlConfig is using dflydev/dot-access-data, which is essentially the direction I had in mind as well.

I agree with you that we should prioritize and stage development. At this point I am uncertain exactly which features go into Robo core. Some applications may not want to use Symfony/Config, while others might. It might be best for Robo to load only the config file or files specified in a --config option, and leave configuration loading policies for specific applications to decide. Or perhaps some default rules (e.g. look in cwd, etc.) might be beneficial. Implementing a loader in BLT first is exactly the right first step; we can decide whether to pull this up to core as things become more defined here.

@jalogut Setter methods that take multiple parameters should have those parameters specified via an array in the configuration items. The only config features that are currently available is a simple Config class that may be injected into command classes that need it by implementing ConfigAwareInterface and using ConfigAwareTrait.

jalogut commented 7 years ago

Hi @daften ,

Yes, I also checked this extension before writing my comment. I was also trying to find a way to overwrite properties from the command line.

One question regarding your extension. After reading the properties, can these properties be accessed from all tasks or only on this property task?

greg-1-anderson commented 7 years ago

Over in #552 I implemented the basics of configuration injection. With this PR, you load your configuration data however you want, to produce a nested array of configuration values, and then inject it into your \Robo\Config object. This could happen in @hook init in a RoboFile via \Robo\Robo::config()->import($data);. (You might want to use extend() instead of import() to take advantage of any not-yet-available Robo configuration loading features that may be added in the future.) Applications that use Robo as a framework typically instantiate their own \Robo\Config object.

Config key remapping is not implemented, and task setter config injection is not implemented, but it's a fairly solid start.

Next step is to get feedback on whether this meets the needs of BLT, bild, digipolisgent/robo-digipolis-general, etc.

jalogut commented 7 years ago

Hi @greg-1-anderson

Thanks for implementing that, it is really a solid start. I will try it out and give you feedback.

greg-1-anderson commented 7 years ago

Over on #552, Robo is now loading simple .yml config files from robo.yml in the same directory as the RoboFile. grasmash/yaml-expander is being used, so variable substitution may be used. RoboFiles that wish to do their own configuration loading are encouraged to do so in their __construct() method. (@hook init is too late.) I'm not using Grasmesh's YamlConfig class directly, but my implementation is very similar, and also uses dflydev/dot-access-data, so the influence is clear here.

It is also now possible to set arbitrary configuration values from the commandline via -D config.key=value. You may have as many of these on the commandline as you'd like. In general, commands should define options for settings so that they are documented in the command help; however, using a configuration value might be preferable in some instances, e.g. for ssh port settings and so on. This meets @jalogut's use-case mentioned above.

Robo has split the configuration implementation up into a Config class and a couple of ConfigLoader classes. Applications that use Robo as a framework may choose to use a different config loader if they wish; for example, Symfony/Config works well here, for anyone working with Symfony bundles, loading config from xml files, or validating config files.

Config key remapping and task setter config injection are still not implemented, but I think those can be put off until after the initial PR is merged, so folks can start using at least the base functionality.

Jelle-S commented 7 years ago

@jalogut In digipolisgent/robo-digipolis-general, all properties are stored in Robo's config, so all tasks that have access to the config object, have access to the properties.

@greg-1-anderson PR looks great so far (AFAICT). Just a side note: How does this code handle config with a dot in the name. For example a yml file that looks like this:

my.config.value: 'value1'
my:
  config:
    value: 'value2'

Which is valid yml, but I dont think dflydev/dot-access-data supports this. We currently use this notation in https://github.com/digipolisgent/robo-digipolis-general/blob/develop/src/DetermineProjectRoot.php#L64 when determining our project and web root. This could (and maybe should) be changed of course, but I thought it was worth noting.

Also, with the current PR, how would you parse multiple robo.yml config files? Our current code checks the entire project root (might be too specific to our use case, but I'd be happy with a configurable directory defaulting to cwd) for default.properties.yml and properties.yml. There is an additional check that these files must be in the same directory as a RoboFile.php. This way projects can define their default properties, which can be overridden by other projects. The properties file in cwd has the highest priority.

grasmash commented 7 years ago

@Jelle-S Good point. I've addressed the issue of expanding dot notated keys in my own implementation here.

Basically, I expand all dot-notated keys in the reference data prior to property expansion, and then expanded dot-notated keys in the new config after property expansion.

<?php
    $reference_data = ArrayManipulator::expandFromDotNotatedKeys($reference_data);
    $file_config = file_exists($yml_path) ? Expander::parse(file_get_contents($yml_path),
      $reference_data) : [];
    $file_config = ArrayManipulator::expandFromDotNotatedKeys($file_config);
    $this->fromArray($file_config);

Pretty straightforward method for expanding dot-notated keys: https://github.com/grasmash/bolt/blob/robo-rebase/src/Robo/Common/ArrayManipulator.php#L46

Full test coverage for the ArrayManipulator class: https://github.com/grasmash/bolt/blob/robo-rebase/tests/phpunit/Robo/ArrayManipulatorTest.php#L86

greg-1-anderson commented 7 years ago

As I mentioned recently in the PR, I don't intend at the moment to support dot-notation in keys in Robo core. I'd like the core implementation to provide a minimum viable set of configuration features, and no more. If you want to lobby for additional convenience features, we'll need to also give @DavertMik to agree. I personally don't think this is the way to go, though. In the current design, if you use Robo as a framework, you can load your config however you want, because it is the responsibility of the app to set up the config object. If you are doing this, then you can load configuration from wherever you wish, and prioritize the different sources as necessary. The Robo Config object is decoupled from the config loader to make this easy. If you want to use Symfony/Config, for example, those classes are already designed to load config supporting all of the features Symfony provides, and return a simple php nested array. This can then be dropped into the Robo config object, and you're done.

My thought is that we will have specific build tools such as BLT and Bild that can provide features such as more than one command file, multiple configuration files, dot-notation property key expansion, or whatever you want. Robo core remains focused on the use-case of providing a single RoboFile that defines build scripts for one specific application.

If you are using Robo core in this way, it will still be possible to have some influence over how configuration is set up. I'm planning on refactoring the PR slightly to make this easier. In your RoboFile's constructor, you will be passed in the unloaded Robo config object. At this time, you will be able to inject your default configuration (from code, or via your own loaders). After your constructor is finished, Robo will load configuration from its sources (currently, only robo.yml at the cwd). These values will take precedence over whatever values you provide in your constructor. To provide configuration that has a higher priority than Robo's default locations, you can add more files / values from the @hook init method of your RoboFile.

We should discuss whether Robo core should load config from more locations than the cwd (e.g. maybe ~/.robo/robo.yml or wherever).

If anyone wishes to discuss how a single RoboFile can load tasks not provided by Robo core, we should start a separate issue to discuss the planned plugin design.

grasmash commented 7 years ago

@greg-1-anderson I would consider this issue addressed.

consolidation / robo