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

Pipe it to connections (#73688) #73914

Merged
merged 1 commit into from
Sep 3, 2021
Merged

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Mar 15, 2021

  • pipelining tweaks
    added 'defaults' entry for ini pipelining from ssh plugin

(cherry picked from commit 9690512)

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

connection/*

* pipelining tweaks
  added 'defaults'  entry for ini pipelining from ssh plugin

(cherry picked from commit 9690512)
@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 backport This PR does not target the devel branch. 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. windows Windows community labels Mar 15, 2021
@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 Mar 25, 2021
@yan12125
Copy link
Contributor

The fix is still not included in ansible-base 2.10.8. Could it be merged before 2.10.9?

Comment on lines -98 to -100
- section: connection
- section: defaults
key: pipelining
- section: ssh_connection
Copy link
Member

Choose a reason for hiding this comment

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

Maybe ssh_connection should stay in the middle for 2.10, just in case someone had the setting specified there and working? I'm +1 for all the rest of the changes here for 2.10, but removing that ability in a bugfix patch might break some folks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nitzmahone the same entries work in the plugin itself, this is not used so its just redundant

Copy link
Member

Choose a reason for hiding this comment

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

Right, but a previous pipelining entry under ssh_connection would have enabled it globally (including for local), yeah? So changing that behavior in a backport patch is probably not the right thing...

Copy link
Member Author

Choose a reason for hiding this comment

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

it still does for plugins that declare it, like paramiko, for most others it doesn't matter

Copy link
Member

Choose a reason for hiding this comment

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

... and that makes total sense going forward, but removing the value in a backport is potentially introducing breaking behavior, because the existing behavior in those versions was global, not per-plugin. It's a corner case, but also one that's easily rectified by just leaving ssh_connection in the middle of the global mix for 2.10.

Copy link
Member Author

Choose a reason for hiding this comment

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

i had checked back into even 2.9, no plugin used this directly

@nitzmahone nitzmahone merged commit aa08dc6 into ansible:stable-2.10 Sep 3, 2021
@bcoca bcoca deleted the pipeit_b branch September 7, 2021 13:32
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Sep 10, 2021
@ansible ansible locked and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 backport This PR does not target the devel branch. bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. has_issue stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants