Skip to content

Fix import for WinRM version check#68768

Open
lubinatien wants to merge 2 commits intosaltstack:3006.xfrom
lubinatien:patch-1
Open

Fix import for WinRM version check#68768
lubinatien wants to merge 2 commits intosaltstack:3006.xfrom
lubinatien:patch-1

Conversation

@lubinatien
Copy link

@lubinatien lubinatien commented Feb 25, 2026

PyPI's package is named pywinrm.

The importlib.metadata.version("winrm") call raises PackageNotFoundError which isn't caught by except ImportError, crashing the block and leaving winrm undefined despite being importable.

What does this PR do?

What issues does this PR fix or reference?

Fixes salt-cloud with Windows minions using Winrm

Previous Behavior

salt-cloud crashes when using winrm to deploy windows minion.

New Behavior

salt-cloud deploys windows minion correctly.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

% cat test.py
import salt.utils.versions

# Set the minimum version of PyWinrm.
WINRM_MIN_VER = "0.3.0"

try:
    import importlib
    import importlib.metadata

    # Verify WinRM 0.3.0 or greater

    version = importlib.metadata.version("winrm")
    if not salt.utils.versions.compare(version, ">=", WINRM_MIN_VER):
        HAS_WINRM = False
    else:
        HAS_WINRM = True

    import winrm
    from winrm.exceptions import WinRMTransportError

except ImportError:
    HAS_WINRM = False

print(HAS_WINRM)
% /opt/saltstack/salt/bin/python3.10 test.py
False
% sed -i 's/importlib\.metadata\.version("winrm")/importlib.metadata.version("pywinrm")/' test.py
% /opt/saltstack/salt/bin/python3.10 test.py
True

Commits signed with GPG?

Yes/No

PyPI's package is named pywinrm.

The importlib.metadata.version("winrm") call raises PackageNotFoundError which isn't caught by except ImportError, crashing the block and leaving winrm undefined despite being importable.
@lubinatien lubinatien requested a review from a team as a code owner February 25, 2026 09:47
@welcome
Copy link

welcome bot commented Feb 25, 2026

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject.pdl@broadcom.com. We're glad you've joined our community and look forward to doing awesome things with you!

twangboy
twangboy previously approved these changes Feb 25, 2026
@twangboy twangboy added the test:full Run the full test suite label Feb 25, 2026
@twangboy twangboy added this to the Sulpher v3006.24 milestone Feb 25, 2026
@twangboy
Copy link
Contributor

Could we get a changelog and a test for this. Maybe just verify that "HAS_WINRM" is true.

@lubinatien
Copy link
Author

Yes of course, I didn't include it as it's a partial rollback to what is was before:
1bb7749
I won't be able to do it now, but will provide you with a test before tomorrow!

@lubinatien
Copy link
Author

lubinatien commented Feb 26, 2026

Ok, hope this will be enough, I won't have time to create a proper test but updated the ticket with what I could.
Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants