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

Replace hardcoded /etc/chef with Chef::Dist::CONF_DIR #9060

Merged
merged 7 commits into from Dec 9, 2019

Conversation

bobchaos
Copy link
Contributor

@bobchaos bobchaos commented Nov 5, 2019

Signed-off-by: Marc Chamberland chamberland.marc@gmail.com

Description

More work towards #8376

This one replaces the last known occurrences that need addressing of /etc/chef with a distro constant in chef-config.

I validated there's no more occurrences of /opt/chef too. Windows paths could also be addressed in this PR (another commit), or another PR if maintainers prefer that.

Related Issue

#8376
#9094

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • [NA] I have updated the documentation accordingly.
  • [NA] I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@bobchaos bobchaos requested review from a team as code owners November 5, 2019 15:41
@@ -33,6 +33,7 @@
require "addressable/uri" unless defined?(Addressable::URI)
require "openssl" unless defined?(OpenSSL)
require "yaml"
require "chef/dist"
Copy link
Contributor

Choose a reason for hiding this comment

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

this gives us a circlular dep between chef-config and chef itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point; I'll add a dist.rb to chef-config to avoid that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See latest commit for proposed fix, but this has me wondering if dist.rb shouldn't simply be entirely owned by chef-config. Some quick and dirty sed could address that if we want to try it.

@bobchaos
Copy link
Contributor Author

bobchaos commented Nov 5, 2019

the windows tests look to have been cancelled. Did we reach some sort of execution timeout?

@tas50
Copy link
Contributor

tas50 commented Nov 5, 2019

Personally long term we should move the dist stuff into chefconfig and then we can rely on it in other products like Ohai which use chef config and may need to output log messages with branding

@bobchaos
Copy link
Contributor Author

bobchaos commented Nov 6, 2019

Personally long term we should move the dist stuff into chefconfig and then we can rely on it in other products like Ohai which use chef config and may need to output log messages with branding

When this is merged I can look into that, I want to try and finish up chef/chef ASAP to minimize the number of times we'll need to adjust CinC's pipelines.

About those Windows tests, should I worry?

@lamont-granquist
Copy link
Contributor

You need to address /var/chef as well, and you're going to need to do something about the hacktastic way that the routine in the other PR matches on the chef substring. That whole platform_specific_path routine is making me very grouchy the way its brittle against that substring. All the relevant configs probably need to do a windows? ? "c:/chef" : "/etc/chef" in them directly at the very least to set their defaults. Most likely the best way would be to be able to specify the default paths for unix and windows as separate config options, which were then merged after the config was read and set.

@bobchaos
Copy link
Contributor Author

I've merged dist_windows_service from #9067 in here and the last commit is my proposed solution to the /var/chef and /etc/chef hackery

@bobchaos bobchaos mentioned this pull request Nov 19, 2019
8 tasks
@bobchaos
Copy link
Contributor Author

note to self: run specs on all subprojects too :/

@bobchaos bobchaos force-pushed the dist_conf_dir branch 2 times, most recently from 62c16a4 to 4732014 Compare November 20, 2019 21:24
@bobchaos
Copy link
Contributor Author

this should be ready for another review pass. Do note I left platform_specific_path's definition in the code in case it's called externally somehow, but I could add a deprecation message or something, or just plain remove it if you're all comfortable with that.

Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
Signed-off-by: Marc Chamberland <chamberland.marc@gmail.com>
@tas50 tas50 changed the title replacing hardcoded /etc/chef with Chef::Dist::CONF_DIR Replace hardcoded /etc/chef with Chef::Dist::CONF_DIR Dec 9, 2019
@tas50 tas50 merged commit 3a37b2b into chef:master Dec 9, 2019
@cinc-bot cinc-bot deleted the dist_conf_dir branch December 9, 2019 23:08
@lock
Copy link

lock bot commented Dec 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants