Skip to content

Conversation

@T-o-b-i
Copy link
Contributor

@T-o-b-i T-o-b-i commented Dec 8, 2017

  • the networkservice resource has a new password property - if it is set
    the sshdriver will login with sshpass (needs to be installed) instead
    of the key file

Signed-off-by: Tobi Gschwendtner tg@bloks.de

@Emantor Emantor self-assigned this Dec 10, 2017
@Emantor Emantor self-requested a review December 10, 2017 17:33
Copy link
Member

@Emantor Emantor left a comment

Choose a reason for hiding this comment

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

Whats missing is an update to the NetworkService documentation in doc/configuration.rst, together with my review comments looks good to me 👍

(stdout, stderr, returncode)
"""
complete_cmd = "ssh -x {prefix} {user}@{host} {cmd}".format(
complete_cmd = "ssh -x{prefix} {user}@{host} {cmd}".format(
Copy link
Member

Choose a reason for hiding this comment

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

As above I'm wondering about the change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i found these changes necessary to prevent 2 spaces following each other in the cmd line.
(2 spaces cause split(' ') to add an empty element in the list which will be treated by ssh as the hostname...)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. In that case I would like you too shuffle the self.ssh_prefix += " -o LogLevel=ERROR" line to the top and handle the extra missing space there. Otherwise we would have to touch the get and put functions as well. I tested your changes against a local device, works fine :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damn - i missed put and get.
should work now with one change less ;-)


def on_activate(self):
self.ssh_prefix = "-i {}".format(os.path.abspath(self.keyfile)
self.ssh_prefix = " -i {}".format(os.path.abspath(self.keyfile)
Copy link
Member

Choose a reason for hiding this comment

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

Is this change with the change below necessary? Looks like an unnecessary change to me.

@T-o-b-i T-o-b-i force-pushed the feature/ssh_password branch from c438a81 to 61dd3fb Compare December 10, 2017 18:27
@codecov-io
Copy link

codecov-io commented Dec 10, 2017

Codecov Report

Merging #167 into master will decrease coverage by <.1%.
The diff coverage is 10%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #167     +/-   ##
========================================
- Coverage    51.2%   51.1%   -0.1%     
========================================
  Files          88      88             
  Lines        4943    4949      +6     
========================================
+ Hits         2532    2533      +1     
- Misses       2411    2416      +5
Impacted Files Coverage Δ
labgrid/driver/sshdriver.py 37.5% <0%> (-1.9%) ⬇️
labgrid/resource/networkservice.py 100% <100%> (ø) ⬆️

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 7169c96...8770d78. Read the comment docs.

@T-o-b-i
Copy link
Contributor Author

T-o-b-i commented Dec 10, 2017

was waiting with the documentation till we agreed on the rest ;-)

(stdout, stderr, returncode)
"""
complete_cmd = "ssh -x {prefix} {user}@{host} {cmd}".format(
complete_cmd = "ssh -x{prefix} {user}@{host} {cmd}".format(
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. In that case I would like you too shuffle the self.ssh_prefix += " -o LogLevel=ERROR" line to the top and handle the extra missing space there. Otherwise we would have to touch the get and put functions as well. I tested your changes against a local device, works fine :)

@T-o-b-i T-o-b-i force-pushed the feature/ssh_password branch from 61dd3fb to 15a84a6 Compare December 10, 2017 20:31
Emantor
Emantor previously approved these changes Dec 10, 2017
the networkservice resource has a new password property - if it is set the
sshdriver will login with sshpass (needs to be installed) instead of the key
file

Signed-off-by: Tobi Gschwendtner <tg@bloks.de>
[r.czerwinski@pengutronix.de: fix commit message, documentation and rebase]
Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
@Emantor Emantor merged commit d962b2a into labgrid-project:master Dec 10, 2017
@T-o-b-i T-o-b-i deleted the feature/ssh_password branch December 10, 2017 23:28
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.

3 participants