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

Transport plugin asyncssh #266

Merged
merged 7 commits into from Nov 6, 2022

Conversation

alekshi
Copy link

@alekshi alekshi commented Oct 28, 2022

  1. Fixed issue with dublicated argumnets in asyncssh transport plugin. In case if preferred authentication order (preferred_auth) is provided as part of transport options, asyncssh transport plugin got exception:
    exceptions.ConnectionException: Something unexpected happens and the connection has been failed. Details asyncssh.connection.connect() got multiple values for keyword argument 'preferred_auth'
    Fix has been suggested in the PR
  2. Small suggestion do not to set agent_path = None in asyncssh transport plugin. Asyncssh uses ssh agent from OS env vars by default and doesn't use at all in case if agent_path set None.

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Base: 90.45% // Head: 90.40% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (31053e1) compared to base (b801217).
Patch coverage: 25.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #266      +/-   ##
==========================================
- Coverage   90.45%   90.40%   -0.06%     
==========================================
  Files          57       57              
  Lines        3196     3198       +2     
==========================================
  Hits         2891     2891              
- Misses        305      307       +2     
Impacted Files Coverage Δ
scrapli/transport/plugins/asyncssh/transport.py 57.57% <25.00%> (-1.19%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@carlmontanari
Copy link
Owner

👋 hey again!

I just pushed two commits -- the first one just appeased mypy and has no actual changes.

The second is basically what I mentioned in the issue. I'm not married to this pattern, but I had a minute so figured id just push that up there so we can discuss! Let us know what ya think!

Carl

Ps - going to go close those issue now and we can just follow up here if that works!

sashashi added 2 commits November 1, 2022 10:24
…transport options (either empty tuple to pick up the default one or str pointing to socket explicitly)
@alekshi
Copy link
Author

alekshi commented Nov 1, 2022

Hey! Thank you for the feedback! I'm on your side regarding ssh-agent usage experience so it sounds reasonable for me to provide it as part of transport options. asyncssh expects to get empty tuple to pick up ssh-agent socket from env vars, so I think it will be better to follow asyncssh style here (just to don't confuse a user). We could just leave comment about that in code. What do you think?

@carlmontanari
Copy link
Owner

hey @alekshi sorry for the delay getting back to this. I re-worded some of the comments since I had left some cruft in there! otherwise, LGTM, thanks a bunch for the PR!

Carl

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.

None yet

2 participants