-
Notifications
You must be signed in to change notification settings - Fork 70
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
🤖 Add processhacker.vm #566
Conversation
Closes #565.
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.
It was discussed in #161 to add System Informer instead of Process Hacker. Any reason you need Process Hacker (over System Informer)?
Can you please squash the commits or use "Squash and merge" when merging? We don't want to keep fix
commits in order to have a clean commit history. I also recommend reading about commits good practices (Use imperative mood, capitalize the first word, etc), for example:
|
||
# Delete Desktop shortcut | ||
$desktopShortcut = Join-Path ${Env:Public} "Desktop\Process Hacker 2.lnk" | ||
if (Test-Path $desktopShortcut) { |
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.
[nitpick] This if
is not needed as the next command has the -ea 0
that is the same as -ErrorAction SilentlyContinue
. We have currently some packages with this if and some without. I would remove it for simplicity.
|
||
$shortcutDir = Join-Path ${Env:TOOL_LIST_DIR} $category | ||
$shortcut = Join-Path $shortcutDir "$toolName.lnk" | ||
$executablePath = Join-Path ${Env:ProgramFiles} "\Process Hacker 2\ProcessHacker.exe" -Resolve |
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.
Should we add the tool to path as well?
Install-BinFile -Name $toolName -Path $executablePath
Closing until further notice. Seems like System Informer might be the better option here. |
Automated PR from #565 🚀