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

Improving SSH configuration defaults #4055

Merged

Conversation

giovannipizzi
Copy link
Member

  • look_for_keys is now set to True by default
    (it's also paramiko's default)
  • allow_agent is now set to True by default (on the shell,
    if you have a SSH agent, you expect that it is automatically
    used by ssh)
  • If an empty string is passed as the key_filename, this is not passed
    to paramiko, so it will search for it rather than complaining that
    file '' does not exist.

Fixes #4054

- `look_for_keys` is now set to `True` by default
  (it's also paramiko's default)
- `allow_agent` is now set to `True` by default (on the shell,
  if you have a SSH agent, you expect that it is automatically
  used by ssh)
- If an empty string is passed as the key_filename, this is not passed
  to paramiko, so it will search for it rather than complaining that
  file `''` does not exist.

Fixes aiidateam#4054
@giovannipizzi
Copy link
Member Author

Not sure how to write a test for this - this would need to make many assumptions on how the SSH keys are setup in the test machine (and this can be different on each person's computer), etc. But I think that the change makes sense so maybe it's ok to merge even without explicit (additional) tests, w.r.t. those we already have?

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #4055 into develop will increase coverage by 7.96%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4055      +/-   ##
===========================================
+ Coverage    70.49%   78.45%   +7.96%     
===========================================
  Files          461      461              
  Lines        34077    34079       +2     
===========================================
+ Hits         24020    26733    +2713     
+ Misses       10057     7346    -2711     
Flag Coverage Δ
#django 70.49% <75.00%> (+0.01%) ⬆️
#sqlalchemy 71.35% <75.00%> (?)
Impacted Files Coverage Δ
aiida/transports/plugins/ssh.py 76.39% <75.00%> (+0.09%) ⬆️
aiida/backends/testbase.py 93.40% <0.00%> (+1.89%) ⬆️
aiida/common/hashing.py 95.10% <0.00%> (+1.97%) ⬆️
aiida/manage/manager.py 95.40% <0.00%> (+1.98%) ⬆️
aiida/manage/tests/__init__.py 88.54% <0.00%> (+2.30%) ⬆️
aiida/backends/manager.py 72.42% <0.00%> (+5.75%) ⬆️
aiida/cmdline/params/types/strings.py 78.85% <0.00%> (+5.77%) ⬆️
aiida/cmdline/params/types/profile.py 66.67% <0.00%> (+8.34%) ⬆️
aiida/orm/implementation/querybuilder.py 79.05% <0.00%> (+8.39%) ⬆️
... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d39ec6a...1974b53. Read the comment docs.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @giovannipizzi . I agree that this doesn't require a test

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.

Improve default ssh configuration of computers
2 participants