Skip to content

Conversation

TinaMor
Copy link
Contributor

@TinaMor TinaMor commented Jun 9, 2025

PR Description

This PR ensures uninstall routines no longer exit prematurely on empty directories.

Current implementation:

if (Test-EmptyDirectory -Path $path) {
Write-Output "$tool does not exist at $Path or the directory is empty"
return
}

This causes the command to exit prematurely, even when the -purge flag is passed.

Additionl tasks:

  1. Properly quotes executable paths for command invocations.
  2. Refactors HNS module import logic
  3. Use $TestDrive instead of TestDtivr:\ in Buildkit Tests
  4. Update unit tests

Checklist

As part of our commitment to engineering excellence, before submitting this PR, please ensure:

  • You have tested this code in both Desktop and Server environments, as well as AMD64 and ARM64 environments (functional testing).
  • You have added unit tests for new code.
  • You have added or updated documentation in the cmdlet docs, command-reference.md, and the modules help files.
  • You have reviewed the PR/code best practices defined in the CONTRIBUTING.md.

In addition, after this PR has been reviewed, please ensure:

  • If you make changes while addressing review comments, you test the final version again in both AMD64 and ARM64 environments.
  • You validate that your changes have not introduced any regressions.

@TinaMor TinaMor changed the title Fix uninstall command Fix uninstall command premature exit Jun 9, 2025
@TinaMor TinaMor requested a review from Copilot June 10, 2025 15:42
Copilot

This comment was marked as outdated.

@TinaMor TinaMor force-pushed the tinamor/fix-uninstall-command branch 3 times, most recently from 9258465 to ddfdf97 Compare June 16, 2025 12:21
@TinaMor TinaMor requested a review from Copilot June 16, 2025 12:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures uninstall routines no longer exit prematurely on empty directories and properly quotes executable paths for command invocations. It also refactors HNS module import logic and updates unit tests to mock the new patterns.

  • Allow -Purge logic to run even when no binaries are found by removing early exit checks.
  • Locate and quote executable paths (nerdctl.exe, containerd.exe, buildkitd.exe) before invoking commands.
  • Refactor HNS import to use Get-Module and pipeline import, and adjust related tests.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
containers-toolkit/Public/NerdctlTools.psm1 Removed empty‐dir guard, added executable lookup and pruning flags
containers-toolkit/Public/ContainerdTools.psm1 Removed early exit, quoted executable paths, updated uninstall helper
containers-toolkit/Public/ContainerNetworkTools.psm1 Refactored HNS import via Get-Module pipeline
containers-toolkit/Public/BuildkitTools.psm1 Quoted commands, replaced service setup warning message
Tests/NerdctlTools.Tests.ps1 Mocked executable lookup, updated prune argument
Tests/ContainerNetworkTools.Tests.ps1 Adjusted expectations around Get-Module and Import-Module
Tests/BuildkitTools.Tests.ps1 Introduced CleanupTestDrive, cleaned up duplicated paths
Comments suppressed due to low confidence (2)

containers-toolkit/Public/BuildkitTools.psm1:143

  • [nitpick] Use "set up" (verb) instead of "setup" for verb usage: e.g., "Failed to set up Buildkitd service. $_".
Write-Warning "Failed to setup Buildkitd service. $_"

Tests/BuildkitTools.Tests.ps1:35

  • The hard-coded path "TestDrive:\" duplicates the $TestDrive variable. Consider using "$TestDrive" consistently to avoid path mismatches and simplify cleanup logic.
Get-ChildItem -Path "TestDrive:\\" -Recurse -Force -ErrorAction SilentlyContinue | `

@TinaMor TinaMor force-pushed the tinamor/fix-uninstall-command branch 2 times, most recently from dc11c78 to b077887 Compare June 16, 2025 12:47
@TinaMor TinaMor force-pushed the tinamor/fix-uninstall-command branch from b077887 to 2fbc4c7 Compare June 16, 2025 13:02
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.

1 participant