Access config in custom Conditions

[Wirone]

Since custom was introduced in the config section, it looks like a nice place for having values for fine-tuning the process of hooks. Default values can be provided in captainhook.json and users can override these in captainhook.config.json. I would like to use this approach to allow fine-tuning of what's happening in the hooks (opt-in / opt-out from checks via config).

Unfortunately, I don't see a way to access Config within condition through established contract (like in Action, which receives it via execute() method). I can use \CaptainHook\App\Config\Factory::create() but it's marked as @internal, so it can disappear/change at any point, so it's not good to use it.

Is it possible to provide access to the config within CaptainHook\App\Hook\Condition instance? Or maybe there is some way that I overlooked? 🙂

[sebastianfeldmann]

Providing access would be possible by changing the Condition interface what would be a breaking change and something for a major version update (not my favorite solution).
We could introduce a ConfigAware interface that forces Conditions to implement the setConfig method so the ConditionRunner could check if a condition is ConfigAware and provide the Config class to the condition.
This would need some further changes because for now even the ConditionRunner is not aware of the configuration.

I'm not sure providing access is to the Config is a good idea yet. The "plan" was to keep Conditions very simple and straight forward.

Do you have an example what you would like to configure for a condition?

Maybe a custom Action is more suited?

[Wirone]

Example scenario: action invoked on post-change hook that refreshes environment (basically composer install && php bin/console cache:clear), so app is ready just after branch checkout. Not every developer wants this to happen automatically, so we need opt-out / opt-in flags. Currently we do this within custom condition class, which reads captainhook.local.json (which is included in the main config) and looks for values stored under specific path (not handled by Captain Hook itself). I wanted to move it to custom config field in order to utilise built-in override system through captainhook.config.json, and keep captainhook.local.json only for extending actions' set for the hooks. I have it working now, but through mentioned @internal method and it would be great if I could use built in contract 🙂. I believe I could make custom action that executes these commands internally and just do nothing when configured to skip, but it make the flow less readable for me, because in the output it would be marked as done even though it did nothing - marking it as skipped is much better, but it needs to be done via condition. Not to mention it would require to create custom Action class for any other "configurable" commands, comparing to reusing one Condition class with different args.

Technically I suspected that it would be a BC break, also currently available IO and Repository passed to isTrue don't look like a good fit to provide Config through them (which wouldn't be a BC break). ConfigAware looks good to me 👍.

[sebastianfeldmann]

What do you think about something like this

            {
                "action": "my-action-bin --some-cool-option",
                "conditions": [
                    {
                        "exec": "\\CaptainHook\\App\\Hook\\Condition\\DecideByArg",
                        "args": [
                            "{$CONFIG|value-of:custom>>should-we-execute-my-action|default:true}"
                        ]
                    }
                ]
            }

The only thing we would have to add is the argument parsing for the Condition setup.
Feels a bit dirty to me but would be less invasive I think.

If we would provide Config access to the Condition itself we could build something like

                        "exec": "\\CaptainHook\\App\\Hook\\Condition\\DecideByCustomConfigValue",
                        "args": [
                            "default-value true|false",
                            "should-we-execute-my-action"
                        ]

The first on is a bit weird the second one a bit more invasive.

What do you think?

[Wirone]

Honestly I prefer the latter, because the former's syntax is a little bit too complex, and I am afraid it could be more error-prone (custom config keys would have been chosen carefully to not brake the syntax, comparing to any naming in the latter). With Config having access to Condition it would be possible to use built-in simple condition(s), but also build your own, more complex ones. One little detail, I think it should be:

                        "exec": "\\CaptainHook\\App\\Hook\\Condition\\IfCustomConfigValueIs[Truthy|Falsy]",
                        "args": [
                            "should-we-execute-my-action",
                            "true|false",
                        ]

and implementation should utilise filter_var($shouldExecute, FILTER_VALIDATE_BOOL) so it's possible to use 0/1, yes|no, true|false in the config. Depending on custom config params' naming it would act as an opt-in or opt-out:

                [
                    {
                        "exec": "\\CaptainHook\\App\\Hook\\Condition\\IfCustomConfigValueIsTruthy",
                        "args": [
                            "REFRESH_RUNTIME_ON_CHECKOUT"
                        ]
                    },
                    {
                        "exec": "\\CaptainHook\\App\\Hook\\Condition\\IfCustomConfigValueIsFalsy",
                        "args": [
                            "SKIP_RUNTIME_REFRESH"
                        ]
                    }
                ]

Second argument could be optional, with default values as true for Truthy, and false for Falsy, to run task by default. As an alternative, second argument could be dropped completely so default custom config value would be enforced on config level (Condition::isTrue() returning false when value is missing in resolved configuration).

What do you think?

PS. Sorry for the late response, I had busy days lately 😅.

[sebastianfeldmann]

No worries, I like the second approach better as well.

I will think a bit more about the way how to grant Config access to Conditions but will most likely start implementing this in the upcoming days.

[sebastianfeldmann]

I just released version 5.17.2 with both new Conditions.

"action": "some-action"
  "conditions": [
    {"exec": "\\CaptainHook\\App\\Hook\\Condition\\Config\\CustomValueIsTruthy",
     "args": [
       "NAME_OF_CUSTOM_VALUE"
    ]}
  ]

"action": "some-action"
  "conditions": [
    {"exec": "\\CaptainHook\\App\\Hook\\Condition\\Config\\CustomValueIsFalsy",
     "args": [
       "NAME_OF_CUSTOM_VALUE"
    ]}
  ]

You can now build your own by implementing the CaptainHook\App\Hook\Condition\ConfigDependant interface.

Hope it helps :)

[Wirone]

Thank you 🍻!