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

We should be loading default config using workstation config loader #153

Merged
merged 1 commit into from
Jun 12, 2018
Merged

We should be loading default config using workstation config loader #153

merged 1 commit into from
Jun 12, 2018

Conversation

tyler-ball
Copy link
Contributor

This must have gotten deleted at some point. Adding it back ensures we load any local ~/.chef/knife.rb or /etc/chef/client.rb on user's workstation

@tyler-ball tyler-ball requested a review from a team May 18, 2018 22:04
@tyler-ball tyler-ball force-pushed the forgot_workstationconfigloader branch from f1d6aeb to d7a9c6c Compare May 18, 2018 22:10
jonsmorrow
jonsmorrow suggested changes May 19, 2018
View reviewed changes
components/chef-run/lib/chef-run/config.rb Outdated
@@ -130,7 +131,8 @@ def reset
end

config_context :chef do
default(:cookbook_repo_paths, ChefConfig::Config[:cookbook_path])
ChefConfig::WorkstationConfigLoader.new(nil, ChefRun::Log).load
default(:cookbook_repo_paths, [ChefConfig::Config[:cookbook_path]].flatten)
Copy link
Contributor

Choose a reason for hiding this comment

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

SO this is only merging in cookbook paths? I'm not sure what it buys us? Does it hinder us in the future to be pulling in knife.rb and client.rb

@marcparadise
Copy link
Member

marcparadise commented May 19, 2018 •
edited
Loading

Could rogue configuration from a local chef/knife.rb affect what gets produced when we generate the cookbook & create the policy archive?

@tyler-ball
Copy link
Contributor Author

@jonsmorrow @marcparadise Its completely possible that loading local ChefConfig could mess with the Policyfile generation we are doing. The WorkstationConfigLoader will fully populate ChefConfig, so if code in the DK references that it then it will use the loaded ChefConfig.

We can get around this by reseting the ChefConfig after using the loader to get the existing value of the cookbook repo. But I'm fine with leaving this open until post ChefConf since I have a workaround in the demo.

@tyler-ball tyler-ball force-pushed the forgot_workstationconfigloader branch 2 times, most recently from 11eed05 to ccb0ed5 Compare June 7, 2018 02:11
@tyler-ball
Copy link
Contributor Author

@jonsmorrow @marcparadise Okay, I updated the code to reset the ChefConfig after we use the workstation config loader. This should prevent it from interfering with Policyfile creation later. What do you think about this now?

@tyler-ball
We should be loading default config using workstation config loader
5d5d729 
Signed-off-by: tyler-ball <tyleraball@gmail.com>
@tyler-ball tyler-ball force-pushed the forgot_workstationconfigloader branch from ccb0ed5 to 5d5d729 Compare June 12, 2018 18:14
@tyler-ball tyler-ball merged commit a1fb3df into master Jun 12, 2018
@tyler-ball tyler-ball deleted the forgot_workstationconfigloader branch June 12, 2018 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheeseplus cheeseplus cheeseplus approved these changes

@marcparadise marcparadise marcparadise approved these changes

@jonsmorrow jonsmorrow jonsmorrow approved these changes

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
@tyler-ball @marcparadise @cheeseplus @jonsmorrow