-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Packaging: Try to make New-Unix package more readable #5625
Conversation
a5071a0
to
0383540
Compare
2b7e98f
to
093ab75
Compare
restarted appveyor due to web timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments.
tools/packaging/packaging.psm1
Outdated
} | ||
} | ||
# Verify depenecies are installed and in the path | ||
Test-Dependecies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in dependencies
, also in the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
tools/packaging/packaging.psm1
Outdated
Start-NativeExecution { gzip -f $RoffFile } | ||
|
||
$ManFile = Join-Path "/usr/local/share/man/man1" (Split-Path -Leaf $GzipFile) | ||
# Generate gzip of mann file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mann -> man
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
tools/packaging/packaging.psm1
Outdated
[String]$Description, | ||
|
||
[Parameter(Mandatory,HelpMessage='Installer Type')] | ||
[String]$Type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment for the possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
tools/packaging/packaging.psm1
Outdated
[Parameter(Mandatory,HelpMessage='The built and gzipped mann file.')] | ||
[String]$MannGzipFile, | ||
|
||
[Parameter(Mandatory,HelpMessage='The destination of the mann file')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mann -> Man. Also line 770
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
[String]$AfterRemoveScript | ||
) | ||
|
||
$Arguments = @( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider ArrayList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This became unreadable after changing to an array list. I filed this issue to add a feature to PowerShell so I can maintain a similar syntax and use a list:
#5643
tools/packaging/packaging.psm1
Outdated
|
||
if ( ($Environment.IsUbuntu -or $Environment.IsDebian) -and !$Distribution ) | ||
{ | ||
throw "$Distribution is require for a Debian based distribution." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require -> required
d7dd323
to
c48c835
Compare
c48c835
to
c8492be
Compare
throw "Dependency precheck failed!" | ||
} | ||
} | ||
# Verify depenecies are installed and in the path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment.
if ($AfterInstallScript) { | ||
Remove-Item -erroraction 'silentlycontinue' $AfterInstallScript | ||
if ($AfterScriptInfo.AfterInstallScript) { | ||
Remove-Item -erroraction 'silentlycontinue' $AfterScriptInfo.AfterInstallScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Force maybe?
if ($AfterRemoveScript) { | ||
Remove-Item -erroraction 'silentlycontinue' $AfterRemoveScript | ||
if ($AfterScriptInfo.AfterRemoveScript) { | ||
Remove-Item -erroraction 'silentlycontinue' $AfterScriptInfo.AfterRemoveScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Force maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be a follow-up PR. I can address these issue in that PR.
According to @TravisEz13 the remaining changes will be done in the next PR. |
* refactor start-pspackage into functions * [Package] Added instrumentation * [Package] update change log * [Package] Fix distribution parameter in get-dependecies * [Package] fix dependencies * [Package] fix issues with validate script
* refactor start-pspackage into functions * [Package] Added instrumentation * [Package] update change log * [Package] Fix distribution parameter in get-dependecies * [Package] fix dependencies * [Package] fix issues with validate script
PR Checklist
Note: Please mark anything not applicable to this PR
NA
.[feature]
if the change is significant or affectes feature testsPR Summary
Move large chunks of code with-in a single function out into functions to make the function easier to understand.