Don't add the Workstation path if it is already there in chef shell-init #151
Conversation
Signed-off-by: Nikhil Gupta <nikhilgupta2102@gmail.com>
There was a problem hiding this comment.
See comment inline, I think we can do this with less of a change.
Separately, I think that this is a good chance to get some of the testing coverage improved - currently we don't have any tests for ChefCLI::Helpers - which is unfortunate because they underpin parts of chef-clis capabilities.
Could you add a new spec and include tests for omnibus_env? You will likely need to stub out the public methods invoked (omnibus_bin_dir, git_bin_dir, etc) because they mostly rely on omnibus_root- which will explode in omnibus_expand_path if the tests are not run in a place that had an actual omnibus install of chef-cli.
lib/chef-cli/helpers.rb
Outdated
| @@ -117,8 +117,15 @@ def omnibus_env | |||
| @omnibus_env ||= | |||
| begin | |||
| user_bin_dir = File.expand_path(File.join(Gem.user_dir, "bin")) | |||
| path = [ omnibus_bin_dir, user_bin_dir, omnibus_embedded_bin_dir, ENV["PATH"] ] | |||
| path << git_bin_dir if Dir.exist?(git_bin_dir) | |||
| original_path = ENV["PATH"].split(File::PATH_SEPARATOR) | |||
There was a problem hiding this comment.
This would still cause duplicates in the odd case where someone had 1 or 2 of these in their path, but not all three.
But I think we might be able to approach from a different direction - what happens if we assemble our final path as it was, then remove duplicates prior to returning?
There was a problem hiding this comment.
See comment inline, I think we can do this with less of a change.
Separately, I think that this is a good chance to get some of the testing coverage improved - currently we don't have any tests for
ChefCLI::Helpers- which is unfortunate because they underpin parts ofchef-clis capabilities.Could you add a new spec and include tests for
omnibus_env? You will likely need to stub out the public methods invoked (omnibus_bin_dir, git_bin_dir, etc) because they mostly rely onomnibus_root- which will explode inomnibus_expand_pathif the tests are not run in a place that had an actual omnibus install of chef-cli.
yeah, i will add a new spec and include tests for omnibus_env.
There was a problem hiding this comment.
This would still cause duplicates in the odd case where someone had 1 or 2 of these in their path, but not all three.
But I think we might be able to approach from a different direction - what happens if we assemble our final path as it was, then remove duplicates prior to returning?
Got it. Working on it.
nikhil2611
force-pushed
the
ng/chef-cli_issue_93
branch
from
20d3dda
to
40a483d
Compare
nikhil2611
force-pushed
the
ng/chef-cli_issue_93
branch
from
40a483d
to
64029cb
Compare
Signed-off-by: Nikhil Gupta <nikhilgupta2102@gmail.com>
tas50
changed the title
Signed-off-by: Nikhil Gupta nikhilgupta2102@gmail.com
Description
While initializing the shell via
shell-init, this will continue adding workstation locations to the path.Now corrected the
omnibus_envpath. This change will check if the path is present in theENV["PATH"]it will just ignore and if not present then it will add that.Related Issue
Fixes. https://github.com/chef/chef-cli/issues/93