Support "#{ENV['FOO']}" expressions in knife.rb #11
Conversation
| username = os.environ.get('LOGNAME') | ||
| if username is None: | ||
| self.fail('could not read $LOGNAME from environment') | ||
| api = self.load('env_values.rb') |
There was a problem hiding this comment.
Did you mean to commit this env_values.rb too?
| @@ -18,3 +18,10 @@ def test_current_dir(self): | |||
| api = self.load('current_dir.rb') | |||
| path = os.path.join(os.path.dirname(__file__), 'configs', 'test_1') | |||
| self.assertEqual(api.client, path) | |||
|
|
|||
| def test_env_variables(self): | |||
| username = os.environ.get('LOGNAME') | |||
There was a problem hiding this comment.
This would fail on some OSes, probably safer to just forcibly set it to something for the test and then revert back. If you wanted to be extra sexy you could also mock os.environ for this test.
There was a problem hiding this comment.
I thought about that, but it will definitely pass on *NIX/OS X systems, which is probably where most people will run tests and where Travis will run them. I'm happy to fix it if you think that's the best route, though.
There was a problem hiding this comment.
I would at least do the set/unset dance:
old_logname = os.environ.get('LOGNAME')
os.environ['LOGNAME'] = 'pychefenv'
try:
...
finally:
if old_logname is None:
del os.environ['LOGNAME']
else:
os.environ['LOGNAME'] = old_lognameThere was a problem hiding this comment.
I think it'd be better to use a variable that's definitely not going to be used by anything else. Added try/finally to unset it in any event.
…tentially-used vars
|
Looks good, thanks for the patch! |
Support "#{ENV['FOO']}" expressions in knife.rb
Also fixes the other knife.rb test cases, which were failing.