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

Native Chef Helpers functions duplicated as node methods #265

Open
ChefAustin opened this issue Nov 4, 2021 · 5 comments · May be fixed by #268
Open

Native Chef Helpers functions duplicated as node methods #265

ChefAustin opened this issue Nov 4, 2021 · 5 comments · May be fixed by #268

Comments

@ChefAustin
Copy link
Contributor

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
@nmcspadden
Copy link
Contributor

nmcspadden commented Nov 4, 2021 via email

@ChefAustin
Copy link
Contributor Author

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?

@nmcspadden
Copy link
Contributor

nmcspadden commented Nov 4, 2021 via email

@ChefAustin ChefAustin linked a pull request Nov 24, 2021 that will close this issue
Open
@ChefAustin
Copy link
Contributor Author

ChefAustin commented Nov 24, 2021 •
edited
Loading

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.

@erikng
Copy link
Contributor

erikng commented Feb 16, 2022

Yesterday I discovered namespace collisions breaking a lot of our linux logic. facebook/chef-cookbooks#211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Migrate to native Chef helper functions ChefAustin/IT-CPE
3 participants
@nmcspadden @erikng @ChefAustin