Skip to content

winrm - Remove extras lookups#83754

Open
jborean93 wants to merge 2 commits intoansible:develfrom
jborean93:winrm-extras
Open

winrm - Remove extras lookups#83754
jborean93 wants to merge 2 commits intoansible:develfrom
jborean93:winrm-extras

Conversation

@jborean93
Copy link
Copy Markdown
Contributor

@jborean93 jborean93 commented Aug 9, 2024 •
edited by mattclay
Loading

SUMMARY

Removed the extras variable lookups for the winrm connection plugin. All valid options are explicitly documented and will no longer be looked up with the ansible_winrm_* variable names. This should have no impact on people using these variables as all existing options have been explicitly defined and will continue to work as before.

ISSUE TYPE
  • Feature Pull Request
ADDITIONAL INFORMATION

The aim of this PR is to remove the usage of _extras functionality so it can be deprecated and eventually removed.

@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. labels Aug 9, 2024
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Aug 9, 2024
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 9, 2024
@ansibot
Copy link
Copy Markdown
Contributor

ansibot commented Aug 9, 2024

The test ansible-test sanity --test pep8 [explain] failed with 8 errors:

test/units/plugins/connection/test_winrm.py:236:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:240:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:242:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:273:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:277:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:279:11: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:321:20: E201: whitespace after '{'
test/units/plugins/connection/test_winrm.py:344:20: E201: whitespace after '{'

The test ansible-test sanity --test validate-modules [explain] failed with 12 errors:

lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (ca_trust_path) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (cert_key_pem) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (cert_pem) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_delegation) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_hostname_override) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_service) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (message_encryption) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (operation_timeout_sec) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (proxy) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (read_timeout_sec) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (send_cbt) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (server_cert_validation) should be '2.18'. Currently StrictVersion ('0.0')

click here for bot help

@jborean93
Copy link
Copy Markdown
Contributor Author

jborean93 commented Aug 9, 2024 •
edited
Loading

The version_added checks need to be ignored (will go away once merged), these options are not new and have been present in the connection plugin for years. It's now just officially documenting them rather than using the extras variable lookup.

@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Aug 13, 2024
@ansibot
Copy link
Copy Markdown
Contributor

ansibot commented Aug 13, 2024

The test ansible-test sanity --test validate-modules [explain] failed with 12 errors:

lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (ca_trust_path) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (cert_key_pem) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (cert_pem) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_delegation) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_hostname_override) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (kerberos_service) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (message_encryption) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (operation_timeout_sec) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (proxy) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (read_timeout_sec) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (send_cbt) should be '2.18'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/winrm.py:0:0: option-incorrect-version-added: version_added for new option (server_cert_validation) should be '2.18'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 21, 2024
@jborean93
winrm - Remove extras lookups
ac50ee9 
Removed the extras variable lookups for the winrm connection plugin. All
valid options are explicitly documented and will no longer be looked up
with the ansible_winrm_* variable names. This should have any impact of
people using these variables as all existing options have been
explicitly defined and will continue to work as before.
@jborean93
Fix pep8 sanity issues
d306090
@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Sep 3, 2024

version_added is missing

@jborean93
Copy link
Copy Markdown
Contributor Author

jborean93 commented Sep 3, 2024

@webknjaz see #83754 (comment)

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 3, 2024
mattclay
mattclay reviewed Sep 6, 2024
View reviewed changes
Comment thread lib/ansible/plugins/connection/winrm.py
auth but over a HTTP or HTTPS connection respectively.
type: list
elements: string
elements: str
Copy link
Copy Markdown
Member

@mattclay mattclay Sep 6, 2024

Choose a reason for hiding this comment

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

Is there a reason not to use choices to indicate the valid options covered in the description?

bcoca
bcoca reviewed Sep 6, 2024
View reviewed changes
Comment thread lib/ansible/plugins/connection/winrm.py
- The choices available depend on your version of pywinrm
- This can be set to C(basic), C(certificate), C(ntlm), C(kerberos), C(credssp).
- It can also be set to C(plaintext) or C(ssl) but these values will use use C(basic)
auth but over a HTTP or HTTPS connection respectively.
Copy link
Copy Markdown
Member

@bcoca bcoca Sep 6, 2024

Choose a reason for hiding this comment

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

Suggested change
auth but over a HTTP or HTTPS connection respectively.
choices:
basic: User and password are sent in plain text to the target
certificate: Uses certificates
credssp: Credential Security Support Provider, or credential delegation from the local computer to the target
kerberos: Use kerberos 3 party authentication server
ntlm: NT Lan manager authentication
plaintext: Equals using `basic` over HTTP
ssl: The same as `basic` over HTTPS

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 13, 2024
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 25, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattclay mattclay mattclay left review comments

@bcoca bcoca bcoca left review comments

Assignees

No one assigned

Labels

ci_verified Changes made in this PR are causing tests to fail. feature This issue/PR relates to a feature request. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jborean93 @ansibot @webknjaz @mattclay @bcoca @s-hertel