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

fixes for verdi computer configure ssh #1933

Closed
wants to merge 3 commits into from

Conversation

dev-zero
Copy link
Contributor

@dev-zero dev-zero commented Sep 3, 2018

No description provided.

@codecov-io
Copy link

codecov-io commented Sep 3, 2018

Codecov Report

Merging #1933 into develop will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1933      +/-   ##
===========================================
- Coverage    67.28%   67.26%   -0.03%     
===========================================
  Files          320      320              
  Lines        33178    33180       +2     
===========================================
- Hits         22325    22319       -6     
- Misses       10853    10861       +8
Impacted Files Coverage Δ
aiida/transport/plugins/ssh.py 69.39% <100%> (+0.09%) ⬆️
aiida/backends/djsite/globalsettings.py 82.05% <0%> (-5.13%) ⬇️
aiida/backends/djsite/db/models.py 75.4% <0%> (-0.89%) ⬇️
aiida/orm/implementation/sqlalchemy/group.py 88.26% <0%> (+0.51%) ⬆️

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 a48ae14...6c03b0a. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.8%) to 67.829% when pulling 4cc3e27 on dev-zero:ssh-config-cli-fixes into 9ec9445 on aiidateam:develop.

@@ -483,7 +485,7 @@ def open(self):
# Open a SSHClient
connection_arguments = self._connect_args
proxystring = connection_arguments.pop('proxy_command', None)
if proxystring is not None:
if proxystring:
Copy link
Contributor

@espenfl espenfl Oct 11, 2018

Choose a reason for hiding this comment

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

This did not work for me. A change to if proxystring != '' did. However, we should really set it to None if it is not specified.

@@ -483,7 +485,7 @@ def open(self):
# Open a SSHClient
connection_arguments = self._connect_args
proxystring = connection_arguments.pop('proxy_command', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, update this to e.g. proxystring = connection_arguments.pop('proxy_command', '')

Copy link
Contributor

@espenfl espenfl Oct 11, 2018

Choose a reason for hiding this comment

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

But we should really use None. When I was fixing this locally I did not manage to do that due to the fact that we have to leave options for the user that has previously defined a string to set it to None. Since the cli does not suggest the string in the entry field, one cannot delete it and then set None internally. Also, it is not so user friendly to set None in the cli. So maybe we should just stick with an empty string.

@sphuber
Copy link
Contributor

sphuber commented Oct 12, 2018

These problems should have been fixed in #1974. Thanks @dev-zero for the suggestions that @waychal could use for the fix. @espenfl you said that if proxycommand would not work for you, but if the proxycommand really is an empty string, it should in fact work. Could you check out the latest develop and confirm that it now works for you?

@sphuber sphuber closed this Oct 12, 2018
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

6 participants