Skip to content
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 ssh_args and friends back, add var for proxy_command #78826

Merged
merged 5 commits into from Sep 21, 2022

Conversation

sivel
Copy link
Member

@sivel sivel commented Sep 20, 2022

SUMMARY

Add ssh_args and friends back, add var for proxy_command. Fixes #78750

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/plugins/connection/paramiko_ssh.py

ADDITIONAL INFORMATION

@sivel
Copy link
Member Author

sivel commented Sep 20, 2022

There are no tests here, maybe I'll add them? Or maybe not since I'm deprecating them on adding. I should at least test that they work, which I have not done yet.

EDIT: Yes, verified it works

@ansibot ansibot added affects_2.15 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 20, 2022
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 20, 2022
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 20, 2022
@sivel
Copy link
Member Author

sivel commented Sep 20, 2022

version_added test failing is expected. Copied the configs from the ssh plugin, since they originally matched.

@ansibot
Copy link
Contributor

ansibot commented Sep 21, 2022

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

lib/ansible/plugins/connection/paramiko_ssh.py:0:0: option-incorrect-version-added: version_added for new option (ssh_args) should be '2.15'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/paramiko_ssh.py:0:0: option-incorrect-version-added: version_added for new option (ssh_common_args) should be '2.15'. Currently StrictVersion ('0.0')
lib/ansible/plugins/connection/paramiko_ssh.py:0:0: option-incorrect-version-added: version_added for new option (ssh_extra_args) should be '2.15'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 21, 2022
deprecated:
why: In favor of the "proxy_command" option.
version: "2.18"
alternatives: proxy_command
Copy link
Member

@bcoca bcoca Sep 21, 2022

Choose a reason for hiding this comment

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

we might want also _paramiko_ versions for each var/env var/ini entry that has higher precedence, JIC people use both connections but need different configuration

Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's already a paramiko-specific option, could the priority just be reversed?

diff --git a/lib/ansible/plugins/connection/paramiko_ssh.py b/lib/ansible/plugins/connection/paramiko_ssh.py
index 82eae5c77e..5410b12b1b 100644
--- a/lib/ansible/plugins/connection/paramiko_ssh.py
+++ b/lib/ansible/plugins/connection/paramiko_ssh.py
@@ -287,7 +287,7 @@ class Connection(ConnectionBase):
             if proxy_command:
                 break
 
-        proxy_command = proxy_command or self.get_option('proxy_command')
+        proxy_command = self.get_option('proxy_command') or proxy_command

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, although that logic existed before this PR, but probably makes sense.

Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

It looks like the vyos incidental tests pass if the ansible.builtin NetworkConnectionBase also adds the get_options fix from ansible.netcommon ansible-collections/ansible.netcommon@130c49e.

@sivel
Copy link
Member Author

sivel commented Sep 21, 2022

It looks like the vyos incidental tests pass if the ansible.builtin NetworkConnectionBase also adds the get_options fix from ansible.netcommon ansible-collections/ansible.netcommon@130c49e.

Hrm, weird, I tested locally and at least the one target was passing. Will look into this.

@s-hertel
Copy link
Contributor

@sivel Oh, maybe it was just a flaky CI error in that case.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 21, 2022
@sivel
Copy link
Member Author

sivel commented Sep 21, 2022

Super weird. First, I was confused, since #78789 is what I thought this PR was for a moment. Second, I'm now confused by why this change causes issues for the vyos tests, I'll have to let them run again now that I pushed another commit.

Maybe this PR just will need to be dependent on #78789, or I need to make the change you recommended.

@sivel
Copy link
Member Author

sivel commented Sep 21, 2022

Alright, must have just been flaky. vyos tests passed this time.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 21, 2022
@sivel sivel merged commit 1b47def into ansible:devel Sep 21, 2022
sivel added a commit to sivel/ansible that referenced this pull request Sep 21, 2022
ansible#78826)

Fixes ansible#78750
(cherry picked from commit 1b47def)

Co-authored-by: Matt Martz <matt@sivel.net>
sivel added a commit to sivel/ansible that referenced this pull request Sep 21, 2022
ansible#78826)

Fixes ansible#78750
(cherry picked from commit 1b47def)

Co-authored-by: Matt Martz <matt@sivel.net>
sivel added a commit to sivel/ansible that referenced this pull request Sep 21, 2022
ansible#78826)

Fixes ansible#78750
(cherry picked from commit 1b47def)

Co-authored-by: Matt Martz <matt@sivel.net>
sivel added a commit that referenced this pull request Sep 21, 2022
#78826) (#78836)

* [stable-2.14] Add ssh_args and friends back, add var for proxy_command (#78826)

Fixes #78750
(cherry picked from commit 1b47def)

Co-authored-by: Matt Martz <matt@sivel.net>

* Remove non-backportable changes
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Sep 21, 2022
@ansible ansible locked and limited conversation to collaborators Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.15 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ansible_ssh_common_args ignored in 2.14.0.dev0 (and 2.12, 2.13)
5 participants