IT-CPE
IT-CPE copied to clipboard
facebook
Native Chef Helpers functions duplicated as node methods
I don't understand why some of the native Chef helper functions [1] -- like def linux?, def macos?, def debian_family?, et al -- are being duplicated in fb_helpers, cpe_helpers.
Rather than extending the node object to contain functions already available to the Chef DSL, why not just use the built-in ones?
Frame number: 0/26
From: /etc/chef/local-mode-cache/cache/cookbooks/cpe_my_org_init/recipes/default.rb:17 Chef::Mixin::FromFile#from_file:
12: # of patent rights can be found in the PATENTS file in the same directory.
13: #
14: require 'pry'
15: binding.pry
16: # Start an empty run_list so we can append to it
=> 17: run_list = []
18:
19: # All machines
20: run_list += [
21: 'global_base',
22: 'my_org_base',
[1] pry(#<Chef::Recipe>)> show-method linux?
From: /opt/chef/embedded/lib/ruby/gems/3.0.0/gems/chef-utils-17.7.29/lib/chef-utils/dsl/os.rb:28:
Owner: ChefUtils::DSL::OS
Visibility: public
Signature: linux?(node=?)
Number of lines: 3
def linux?(node = __getnode)
node["os"] == "linux"
end
[2] pry(#<Chef::Recipe>)> show-method node.linux?
From: /etc/chef/local-mode-cache/cache/cookbooks/fb_helpers/libraries/node_methods.rb:50:
Owner: Chef::Node
Visibility: public
Signature: linux?()
Number of lines: 3
def linux?
self['os'] == 'linux'
end
Can anyone explain why this is necessary/beneficial?
[1] See: https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/os.rb & https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/platform.rb & https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/platform_family.rb
The darwin? one, for example, was added in 15.5: https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/os.rb#L47
These predate nearly all of the built-in helper functions. We've just never migrated because it didn't make a difference.
On Thu, Nov 4, 2021 at 7:35 AM Austin Culter @.***> wrote:
I don't understand why some of the native Chef helper functions [1] -- like def linux?, def macos?, def debian_family?, et al -- are being duplicated in fb_helpers, cpe_helpers.
Rather than extending the node object to contain functions already available to the Chef DSL, why not just use the built-in ones?
Frame number: 0/26
From: /etc/chef/local-mode-cache/cache/cookbooks/cpe_my_org_init/recipes/default.rb:17 Chef::Mixin::FromFile#from_file:
12: # of patent rights can be found in the PATENTS file in the same directory. 13: # 14: require 'pry' 15: binding.pry 16: # Start an empty run_list so we can append to it=> 17: run_list = [] 18: 19: # All machines 20: run_list += [ 21: 'global_base', 22: 'my_org_base',
[1] pry(#Chef::Recipe)> show-method linux?
From: /opt/chef/embedded/lib/ruby/gems/3.0.0/gems/chef-utils-17.7.29/lib/chef-utils/dsl/os.rb:28: Owner: ChefUtils::DSL::OS Visibility: public Signature: linux?(node=?) Number of lines: 3
def linux?(node = __getnode) node["os"] == "linux" end [2] pry(#Chef::Recipe)> show-method node.linux?
From: /etc/chef/local-mode-cache/cache/cookbooks/fb_helpers/libraries/node_methods.rb:50: Owner: Chef::Node Visibility: public Signature: linux?() Number of lines: 3
def linux? self['os'] == 'linux' end
Can anyone explain why this is necessary/beneficial?
[1] See: https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/os.rb & https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/platform.rb & https://github.com/chef/chef/blob/main/chef-utils/lib/chef-utils/dsl/platform_family.rb
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/facebook/IT-CPE/issues/265, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJFTX6VT4DYQ4LGDMT56MLUKKKZ7ANCNFSM5HLS6RBA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
--
Nick McSpadden @.***
Would you entertain a PR to remove them? Or is your fleet of devices running pre-15.5 client versions and such a PR would break them?
We likely wouldn't accept that PR without pretty thorough testing in our environment. I'd encourage you to put one up to kickstart the conversation, but migrating that is also pretty low-pri unless there's a demonstrable difference in performance/time - or some other indication of problem that this is causing.
On Thu, Nov 4, 2021 at 8:51 AM Austin Culter @.***> wrote:
Would you entertain a PR to remove them? Or is your fleet of devices running pre-15.5 client versions and such a PR would break them?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/facebook/IT-CPE/issues/265#issuecomment-961176396, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJFTX3H6EKZXKBSUL5TRLDUKKTZVANCNFSM5HLS6RBA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
--
Nick McSpadden @.***
We likely wouldn't accept that PR without pretty thorough testing in our environment. I'd encourage you to put one up to kickstart the conversation, but migrating that is also pretty low-pri unless there's a demonstrable difference in performance/time - or some other indication of problem that this is causing.
PR opened. Let me know if you have any concerns or discover any issues when you get around to testing internally.
Yesterday I discovered namespace collisions breaking a lot of our linux logic. https://github.com/facebook/chef-cookbooks/issues/211
