Skip to content

Add -TemporaryPath parameter to Install-PSResource, Save-PSResource, and Update-PSResource #763

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

Merged
merged 10 commits into from
Aug 22, 2022
Merged

Conversation

alyssa1303
Copy link
Contributor

PR Summary

This PR adds a parameter called -TemporaryPath for Install-PSResource, Save-PSResource, and Update-PSResource cmdlets, which allows users to set the directory for temporary installation.

PR Context

Resolves #650

PR Checklist

Copy link
Member

@alerickson alerickson left a comment

Choose a reason for hiding this comment

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

Everything looks good-- just have 1 comment on providing user messaging. @anamnavi and @PaulHigin may have some opinions on that as well.

Copy link
Member

@anamnavi anamnavi left a comment

Choose a reason for hiding this comment

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

Not sure about whether we warn/throw/write error if the temp path directory does not exist, but aside from that looks great to me.

alyssa1303 and others added 2 commits August 17, 2022 11:19
Co-authored-by: Anam Navied <anam.naviyou@gmail.com>
Co-authored-by: Anam Navied <anam.naviyou@gmail.com>
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

I also noticed that the InstallHelper class is derived from PSCmdlet, and this is incorrect because it is not being processed as a PowerShell cmdlet. I think this was done so that the PSCmdlet ThrowTerminatingError() and WriteError() methods could be called. But this is incorrect (and I am not sure what the behavior is). InstallHelper should throw a normal C# exceptions and the top level cmdlets should catch and rethrow. Also InstallHelper should return a string[] for any errors to be reported by the top level cmdlet.

@alyssa1303 Can you make these changes, or create a new Issue to make these changes later (since this is not really in scope of this PR)?

@anamnavi
Copy link
Member

I also noticed that the InstallHelper class is derived from PSCmdlet, and this is incorrect because it is not being processed as a PowerShell cmdlet. I think this was done so that the PSCmdlet ThrowTerminatingError() and WriteError() methods could be called. But this is incorrect (and I am not sure what the behavior is). InstallHelper should throw a normal C# exceptions and the top level cmdlets should catch and rethrow. Also InstallHelper should return a string[] for any errors to be reported by the top level cmdlet.

@alyssa1303 Can you make these changes, or create a new Issue to make these changes later (since this is not really in scope of this PR)?

I think creating an issue for that (and a separate PR) would be better.

Co-authored-by: Paul Higinbotham <paulhi@microsoft.com>
@alyssa1303
Copy link
Contributor Author

I created an issue #767

Copy link
Member

@anamnavi anamnavi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@alyssa1303 alyssa1303 merged commit e799a53 into PowerShell:master Aug 22, 2022
anamnavi pushed a commit that referenced this pull request Sep 7, 2022
Add -TemporaryPath parameter to Install-PSResource, Save-PSResource, and Update-PSResource
@alyssa1303 alyssa1303 added PR-New-Feature This pull request resolves request for a new feature Release This pull request is part the next release labels Sep 22, 2022
@alyssa1303 alyssa1303 removed the Release This pull request is part the next release label Nov 2, 2022
@SydneyhSmith SydneyhSmith removed the PR-New-Feature This pull request resolves request for a new feature label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the temp folder for Save-Module configurable
5 participants