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.

Sign up for GitHub

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

Jump to bottom

Login shell #149

Merged
merged 10 commits into from
Sep 20, 2016
Merged

Login shell #149

merged 10 commits into from
Sep 20, 2016

Conversation

jonathanmorley
Copy link
Contributor

Add login and shell options, to be able to force the command to run in a login shell

@jonathanmorley
Copy link
Contributor Author

This is to address #148

@chris-rock
Copy link
Contributor

@jonathanmorley It a great idea to support the login shell. By default, I would expect that train would run in a non-interactive, non-login mode. I would prefer to make this optional.

@jonathanmorley
Copy link
Contributor Author

This change keeps the default behaviour of train (the shell and login options default to false). With login set to true you get a login shell. I haven't looked into how this affects interectiveness

chris-rock
chris-rock reviewed Sep 16, 2016
View reviewed changes
lib/train/extras/command_wrapper.rb
def build_prefix
return '' unless @sudo
return '' if @user == 'root'
def sudo_wrap(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add comments what these function do

lib/train/extras/command_wrapper.rb
@@ -29,6 +29,8 @@ class LinuxCommand < CommandWrapperBase
Train::Options.attach(self)

option :sudo, default: false
option :shell, default: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Its a little bit sub-optimal to switch types of a variable, I would prefer nil here, which would lead to the default

Copy link
Contributor

Choose a reason for hiding this comment

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

Also lets add a comment to give users an idea of potential values

chris-rock
chris-rock reviewed Sep 16, 2016
View reviewed changes
lib/train/extras/command_wrapper.rb
def shell_wrap(cmd)
return cmd unless @shell || @login
login_param = '--login ' if @login
shell = @shell.instance_of?(String) ? @shell : '$SHELL'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to check for the type here

@arlimus
Copy link
Contributor

arlimus commented Sep 20, 2016 •
edited
Loading

Great idea 👍
Thank you @jonathanmorley

chris-rock
chris-rock reviewed Sep 20, 2016
View reviewed changes
.rubocop.yml
@@ -69,3 +69,5 @@ Style/SpaceAroundOperators:
Enabled: false
Style/IfUnlessModifier:
Enabled: false
Style/FrozenStringLiteralComment:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the default setting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because we don't yet have frozen string literal comments in our files:
http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/FrozenStringLiteralComment

I think it's ok to disable for now and let's tackle this in a rubocop cleanup session 😄

@chris-rock
Copy link
Contributor

@jonathanmorley Thanks for incorporating my comments. Could you fix the functional test?

  1) Failure:
linux command::shell wrapping#test_0002_wraps sudo commands in a default shell with login [/home/travis/build/chef/train/test/unit/extras/command_wrapper_test.rb:73]:
--- expected
+++ actual
@@ -1 +1 @@
-"echo '0.7279336654738647' > $SHELL --login"
+"echo 'sudo 0.7279336654738647' > $SHELL --login"

@jonathanmorley
Copy link
Contributor Author

jonathanmorley commented Sep 20, 2016 •
edited
Loading

Fixed. One remaining concern I have here is over quotes in the cmd. I have wrapped the cmd in single quotes so that the outer shell doesn't parse out any special characters, and through testing of things like

sh-3.2$ TEST1=test1 echo 'TEST2=test2; echo $TEST1; echo $TEST2; echo 'quotes'' | /bin/sh

test2
quotes

It seems to work as intended.

However, I see that for sudo we use base64 encoding. Any preferences between the 2 approaches?

@chris-rock
Copy link
Contributor

Thanks for bringing up the question @jonathanmorley base64 proofed to be very reliant so far, therefore I would prefer that solution...

@chris-rock
Copy link
Contributor

💯 Thanks @jonathanmorley for this improvement! This travis errors are caused by docker timeouts not related to this PR

@chris-rock chris-rock merged commit b38f906 into inspec:master Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arlimus arlimus arlimus left review comments

@chris-rock chris-rock chris-rock left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonathanmorley @chris-rock @arlimus @chef-supermarket