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

Issue 1079 bash login shell #1502

Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented May 8, 2018

fixes #1079 , helps with marvel-nccr/quantum-mobile#66

use login shell to execute all remote SSH commands

This has the advantage that all commands will be executed in the same
environment as seen by a user logging in on the machine
(e.g. on most UNIX systems, .profile will be sourced for login shells,
while it won't be sourced for non-login shells).

Additionally, calling bash -l -c '<cmd>' instead of <cmd>
means that the bash shell will be used, independent of the default
shell on the system.
This is advantageous on systems, where bash is not the default shell
(since our remote commands work only in bash).

Also

  • rename doubler code: adding a space, single quote and double quote to check robustness of escaping mechanism in AiiDA

adding a space, single quote and double quote to check robustness
of escaping mechanism in AiiDA
use login shell to execute all remote SSH commands

This has the advantage that all commands will be executed in the same
environment as seen by a user loggin in onto the machine
(e.g. on most UNIX systems, .profile will be sourced for login shells,
while it won't be sourced for non-login shells).

Additionally, calling `bash -l -c '<cmd>'` instead of `<cmd>`
means that the bash shell will be used, independent of the default
shell on the system.
This is advantageous on systems, where bash is not the default shell
(since our remote commands work only in bash).
@ltalirz ltalirz requested a review from giovannipizzi May 8, 2018 20:47
@codecov-io
Copy link

codecov-io commented May 8, 2018

Codecov Report

Merging #1502 into release_v0.12.1 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release_v0.12.1    #1502      +/-   ##
===================================================
+ Coverage            54.42%   54.43%   +<.01%     
===================================================
  Files                  246      246              
  Lines                32478    32480       +2     
===================================================
+ Hits                 17677    17679       +2     
  Misses               14801    14801
Impacted Files Coverage Δ
aiida/transport/plugins/ssh.py 61.92% <100%> (ø) ⬆️
aiida/transport/plugins/local.py 80.6% <100%> (+0.1%) ⬆️

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 d3ebdf1...33ec4fb. Read the comment docs.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

awesome! as this doesn't touch the daemon, it should be easily merge able into develop. shouldn't we do the same for the local transport?
https://github.com/aiidateam/aiida_core/blob/67b26dd3911e68d8f1660f95d9559f9d3dc9d625/aiida/transport/plugins/local.py#L711

@giovannipizzi
Copy link
Member

Thanks!
Good if we could then also merge into develop at some point!

@ltalirz ltalirz merged commit 07c83bc into aiidateam:release_v0.12.1 May 11, 2018
@ltalirz ltalirz deleted the issue_1079_bash_login_shell branch June 26, 2018 11:46
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