Address PR #43 review-thread feedback on warning handling, docs, and release version bump#44
Address PR #43 review-thread feedback on warning handling, docs, and release version bump#44Copilot wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses prior review feedback by refining warning propagation/visualization in the PowerShell GUI flow, improving README prerequisite instructions, and updating the release workflow’s version-bump targeting after script header shifts.
Changes:
- Updates
WingetIntunePackager.ps1to improve GUI connection validation, warning capture/display behavior, and a few robustness tweaks (icon download, module detection). - Adds/clarifies README prerequisites for Entra app registration.
- Adjusts the release workflow’s version-bump
sedtarget to align with the shifted version assignment line.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sources/WingetIntunePackager.ps1 | Adds desktop PowerShell requirement, improves GUI validation and warning capture/display, and hardens a few web/icon/module checks. |
| README.md | Adds a prerequisites section describing Entra app registration steps more clearly. |
| .github/workflows/Create_New_Version.yml | Updates the version-bump step to target the new script version line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Invoke-IntunePackage $Win32AppArgs -WarningVariable warning | ||
|
|
||
| if ([string]::IsNullOrEmpty($warning)) { | ||
| $AppInfo = @() | ||
| $SearchTextBox.Text = "" | ||
| $IDComboBox.Text = "" | ||
| $IDComboBox.Items.Clear() | ||
| $VersionTextBox.Text = "" | ||
| $OverrideTextBox.Text = "" | ||
| $IntuneDescriptionTextBox.Text = "" | ||
| $WhitelistCheckbox.IsChecked = $false | ||
| $AllowUninstallCheckbox.IsChecked = $false | ||
| $InstallUserContextCheckbox.IsChecked = $false | ||
| $CreateButton.IsEnabled = $false | ||
| $AppIcon.Source = $null | ||
| Close-PopUp | ||
| } | ||
|
|
||
| if (-not [string]::IsNullOrEmpty($warning)) { | ||
| Start-PopUp ($warning -join "`n") | ||
| } |
There was a problem hiding this comment.
Fixed in the latest commit. Both checks now use PowerShell truthiness (if (-not $warning) / if ($warning)), and @($warning) -join is used to safely handle arrays of warning messages.
| - name: Update Powershell Version | ||
| #Replace line 12 with new version | ||
| run: sed -i '12s/.*/$Script:WingetIntunePackager = "${{ steps.versioning.outputs.version }}"/g' sources/WingetIntunePackager.ps1 | ||
| #Replace line 14 with new version | ||
| run: sed -i '14s/.*/$Script:WingetIntunePackager = "${{ steps.versioning.outputs.version }}"/g' sources/WingetIntunePackager.ps1 |
There was a problem hiding this comment.
Fixed in the latest commit. The sed command now matches the $Script:WingetIntunePackager variable name pattern rather than a hard-coded line number, so it stays correct regardless of file layout changes.
This PR applies all requested fixes from the linked review thread, focused on warning propagation behavior, documentation typos/clarity, and release workflow correctness after script header changes.
PowerShell warning handling (
sources/WingetIntunePackager.ps1)System.Object[]) by joining captured warnings before display.Add-IntuneWin32Appwarning capture to avoid duplicate/default emission and re-emits each warning exactly once.README prerequisites wording (
README.md)registration,registrations,Leave).Release automation line targeting (
.github/workflows/Create_New_Version.yml)sedline index from 12 to 14 so workflow updates the correct$Script:WingetIntunePackagerassignment after the added#Requiresline.