Skip to content

Make -Path a required parameter for Save-PSResource cmdlet #780

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Make -Path a required parameter for Save-PSResource cmdlet #780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0450de8
change path handle
alyssa1303 Aug 26, 2022
d53a330
delete test
alyssa1303 Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions help/Save-PSResource.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,15 +192,14 @@ Accept wildcard characters: False

### -Path

Specifies the path to save the resource to. If no path is provided, the resource is saved to the
current location.
Specifies the path to save the resource to.

```yaml
Type: System.String
Parameter Sets: (All)
Aliases:

Required: False
Required: True
Position: Named
Default value: None
Accept pipeline input: False
Expand Down
22 changes: 7 additions & 15 deletions src/code/SavePSResource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public sealed class SavePSResource : PSCmdlet
/// <summary>
/// The destination where the resource is to be installed. Works for all resource types.
/// </summary>
[Parameter]
[Parameter(Mandatory = true)]
[ValidateNotNullOrEmpty]
public string Path
{
Expand All @@ -90,21 +90,13 @@ public string Path

set
{
string resolvedPath = string.Empty;
if (string.IsNullOrEmpty(value))
{
// If the user does not specify a path to save to, use the user's current working directory
resolvedPath = SessionState.Path.GetResolvedPSPathFromPSPath(SessionState.Path.CurrentFileSystemLocation.Path).First().Path;
}
else {
resolvedPath = SessionState.Path.GetResolvedPSPathFromPSPath(value).First().Path;
}
if (WildcardPattern.ContainsWildcardCharacters(value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of checking for wildcards here we should try to resolve the path and if that throws, catch and display the error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That way if there's any issues at all with the path we can properly handle and emit the error.

Copy link
Contributor

@PaulHigin PaulHigin Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with wildcards in the path is that they can return multiple matches, and then which path do we use? Whatever we choose will not be apparent to the user, unless we write out a message.

I feel we should not support wildcards in this case, and require the user to provide a single path that resolves to a real location. If not then that is a terminating error. We don't need to test the path and return an error, because GetResolvedPSPathFromPSPath already does this and throws an exception with a readable error:

"Cannot find path 'C:\temp\temp' because it does not exist."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, this makes sense

{
throw new PSArgumentException("Wildcard characters are not allowed in the path.");
}

// Path where resource is saved must be a directory
if (Directory.Exists(resolvedPath))
{
_path = resolvedPath;
}
// This will throw if path cannot be resolved
_path = SessionState.Path.GetResolvedPSPathFromPSPath(value).First().Path;
}
}
private string _path;
Expand Down
2 changes: 1 addition & 1 deletion test/SavePSResource.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ Describe 'Test Save-PSResource for PSResources' {
# Save script that is not signed
# Should throw
It "Save script that is not signed" -Skip:(!(Get-IsWindows)) {
{ Save-PSResource -Name "TestTestScript" -Version "1.3.1.1" -AuthenticodeCheck -Repository $PSGalleryName -TrustRepository } | Should -Throw -ErrorId "GetAuthenticodeSignatureError,Microsoft.PowerShell.PowerShellGet.Cmdlets.SavePSResource"
{ Save-PSResource -Name "TestTestScript" -Version "1.3.1.1" -AuthenticodeCheck -Repository $PSGalleryName -TrustRepository -Path $SaveDir} | Should -Throw -ErrorId "GetAuthenticodeSignatureError,Microsoft.PowerShell.PowerShellGet.Cmdlets.SavePSResource"
}

<#
Expand Down