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

Paths shouldn't be validated when the condition evaluates to false #342

Closed
mikefrobbins opened this Issue Aug 20, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@mikefrobbins
Contributor

mikefrobbins commented Aug 20, 2018

Scenario: I want to create a path for a new module. If the user answers 'yes' to the question of whether or not to create a Git top level folder, they receive an additional prompt for a 'GitRepoName' and that name is used to create an additional top level folder that the module folder is a sub-folder of.

Parameters: Name, Git, GitRepoName

If Git = Yes, then the folder structure should look like this:

\GitRepoName\Name

If Git = No, then it should like like this:

\Name

The problem is that a condition that evaluates to false still validates the path specified in that line of the manifest.

Sample PlasterTemplate.xml file

<?xml version="1.0" encoding="utf-8"?>
<plasterManifest schemaVersion="1.1" templateType="Project" xmlns="http://www.microsoft.com/schemas/PowerShell/Plaster/v1">
   <metadata>
      <name>ScriptModuleTemplate</name>
      <id>e4b0f5db-b8cf-45ab-b0bd-76abfeb72e33</id>
      <version>1.0.0</version>
      <title>Script Module Template</title>
      <description></description>
      <author>Mike F. Robbins</author>
      <tags></tags>
   </metadata>
   <parameters>
      <parameter name="Name" type="text" prompt="Name of the module"/>
      <parameter name="Git" type="choice" default="0" prompt="Create Git top level folder?">
         <choice label="&amp;Yes" value="Yes" help="Create a top level folder for Git with module folder nested in it."/>
         <choice label="&amp;No" value="No" help="No Git top level folder. Module folder will be the root."/>
      </parameter>
      <parameter condition="$PLASTER_PARAM_Git -eq 'Yes'" name="GitRepoName" type="text" prompt="Git repo name for this module? Press enter if not using Git." default="${PLASTER_PARAM_Name}"/>
   </parameters>
   <content>
      <message>Creating folder structure</message>
      <file condition="$PLASTER_PARAM_Git -eq 'Yes'" source="" destination="${PLASTER_PARAM_GitRepoName}\${PLASTER_PARAM_Name}"/>
      <file condition="$PLASTER_PARAM_Git -eq 'No'" source="" destination="${PLASTER_PARAM_Name}"/>
   </content>
</plasterManifest>

Sample code to generate the folder structure:

#Create a new PowerShell Script module
$plasterParams = @{
    TemplatePath      = 'C:\tmp\Plaster'
    DestinationPath   = 'C:\tmp'
    Name              = 'MyModule'
    Git               = 'No'
    GitRepoName       = 'MyRepo'
}
If (-not(Test-Path -Path $plasterParams.DestinationPath -PathType Container)) {
    New-Item -Path $plasterParams.DestinationPath -ItemType Directory | Out-Null
}
Invoke-Plaster @plasterParams

The following error is generated when 'Git' is set to 'No':

The path '\MyModule' specified in the file directive in the template manifest cannot be an absolute path.  Change the path to a 
relative path.
At C:\Program Files\WindowsPowerShell\Modules\Plaster\1.1.3\InvokePlaster.ps1:1075 char:17
+ ...             throw ($LocalizedData.ErrorPathMustBeRelativePath_F2 -f $ ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (The path '\MyMo... relative path.:String) [], RuntimeException
    + FullyQualifiedErrorId : The path '\MyModule' specified in the file directive in the template manifest cannot be an absolute path. 
     Change the path to a relative path.

This is due to the path in the following line being validated even when the condition evaluates to false:

<file condition="$PLASTER_PARAM_Git -eq 'Yes'" source="" destination="${PLASTER_PARAM_GitRepoName}\${PLASTER_PARAM_Name}"/>

The problem can be worked around by prompting the user for a 'GitRepoName' even when it's not used, but this shouldn't be required.

@mikefrobbins

This comment has been minimized.

Contributor

mikefrobbins commented Aug 20, 2018

Line 1067 of the InvokePlaster.ps1 file has this comment:

# We could choose to not check this if the condition eval'd to false
# but I think it is better to let the template author know they've broken the
# rules for any of the file directives (not just the ones they're testing/enabled).

This logic is flawed as the path may not be valid if the condition evaluates to false based on how the template is created as is the case in my scenario.

@TylerLeonhardt

This comment has been minimized.

Collaborator

TylerLeonhardt commented Aug 23, 2018

Thanks for the details @mikefrobbins, it's very helpful! We'll get to this as soon as we can but time has been precious recently. If you feel like you can tackle this, I encourage you to contribute!

@rkeithhill

This comment has been minimized.

Collaborator

rkeithhill commented Aug 23, 2018

Seems like a reasonable request and an easy change to make - just move these lines:


            $condition  = $Node.condition
            if ($condition -and !(EvaluateConditionAttribute $condition "'<$($Node.LocalName)>'")) {
                $PSCmdlet.WriteDebug("Skipping $($Node.localName) '$srcRelPath' -> '$dstRelPath', condition evaluated to false.")
                return
            }

above the comment line you referred to. If you don't have time to submit a PR, I can make the change but if you want to tackle this, that'd be great.

@mikefrobbins

This comment has been minimized.

Contributor

mikefrobbins commented Aug 23, 2018

I ran the debugger on the code before and after the change to see what happens. I see moving it prior to the comment as recommended causes the code to hit the return statement before the error would occur when the condition is false, although it does still seem to do a lot even when a condition is false before it gets to that point.

I've submitted the recommended changes and changed the comment in the code to reflect the new logic.

@mikefrobbins

This comment has been minimized.

Contributor

mikefrobbins commented Aug 23, 2018

Issue resolved in PR #343.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment