microsoft / AL-Go

The plug-and-play DevOps solution for Business Central app development on GitHub
MIT License
253 stars 115 forks source link

Why filter deployment settings? #1109

Closed cegekaJG closed 1 week ago

cegekaJG commented 2 weeks ago

In the action "DetermineDeploymentEnvironments", a step defines a default list of parameters and overwrites them with custom values if they are defined in the configuration.

https://github.com/microsoft/AL-Go/blob/c1020361df06b369c3fed56cefced85e1709f03b/Actions/DetermineDeploymentEnvironments/DetermineDeploymentEnvironments.ps1#L139-L165

Wouldn't it be better, however, to just add every property of the deployment config to the environment settings? That way, it's possible to define custom parameters that can be used by custom deployment scripts. I don't know if there is any code that relies on a known quantity of properties, so I don't see a risk in allowing this.

cegekaJG commented 2 weeks ago

On that note, why pass the parameters of the custom deploy script in a single hashtable? Wouldn't it be much better to use splatting to pass each individual parameter and allow for features like default values and parameter properties?

So, instead of https://github.com/microsoft/AL-Go/blob/c1020361df06b369c3fed56cefced85e1709f03b/Actions/Deploy/Deploy.ps1#L80-L85

use

    $parameters = @{
        "type" = $type
        "AuthContext" = $authContext
        "Apps" = $apps
    } + $deploymentSettings
    . $customScript @parameters
freddydk commented 1 week ago

For custom deployments (or overridden deployments), yes - it would be a good idea to include all properties in the deploymentsettings - will investigate this.

The method of getting all parameters as a hashtable is the same all over - cannot remember the reason behind that design, but it is kind of hard to change now (unless we would have a look at the script and analyze it - which would be possible, but maybe not a must-have).

cegekaJG commented 1 week ago

Is the PS session in strict mode? We could pass both the hashtable and the splatted hashtable to maintain backward compatibility. Future deploy scripts can ignore the parameters hashtable, and legacy ones can ignore the individual parameters.

freddydk commented 1 week ago

Yes, we are running strict mode version 2.0