Update Xcode helpers to match namespace standard #16
Conversation
libraries/xcode.rb
Outdated
| @@ -1,5 +1,5 @@ | |||
| module Xcode | |||
There was a problem hiding this comment.
If we're going to update the namespace, let's match the convention of the rest of the helper modules and put MacOS as the top level and XcodeHelper as the second, where the name that comes before Helper is the name of the resource.
| include Xcode::Helper | ||
|
|
||
| resource_name :xcode | ||
| default_action %i(setup install_xcode install_simulators) |
There was a problem hiding this comment.
The latest simulators are always installed with Xcode. What's the reasoning behind making n-1 the default behavior of the resource?
There was a problem hiding this comment.
That's kind of outside the scope of this change - but the behavior is technically determined by the node attribute default['macos']['xcode']['simulator']['major_version'] = %w(11 10). I can move that attribute to the testing cookbook at some point if you'd prefer.
There was a problem hiding this comment.
Indeed - it's outside the scope. A separate PR should be opened for that change.
libraries/xcode.rb
Outdated
| end | ||
| end | ||
|
|
||
| Chef::Recipe.include(MacOS::XcodeHelpers) |
There was a problem hiding this comment.
Why do we need to have the Xcode helpers available in all recipes and resources?
The docs point to using an action_class for resource actions.
There was a problem hiding this comment.
There's actually a refactor of this using action_class coming up in another branch (xcode-idempotence) - I was hoping to clear this up in the meantime. Right now some of these helper methods are used in downstream cookbooks when further tinkering with Xcode installs.
There was a problem hiding this comment.
In order for helper methods to be used in guards, they must be made available to Chef::Recipe. In order for them to be used "inside" a resource (i.e. a property or part of custom resource logic), they must be included in Chef::Resource.
You are correct about action_class, but that refactor is outside the scope of this PR and upcoming in feature/xcode-idempotence.
There was a problem hiding this comment.
👍 Thank you for reminding me!
There was a problem hiding this comment.
I personally don't like adding these methods directly to the global namespace of resources & recipes. It clutters the namespace, provides poor encapsulation, and forces the use of longer than needed method names in order to clarify intent. It's much cleaner if the call site can read something like Xcode.installed? rather than a global method xcode_installed?.
libraries/xcode.rb
Outdated
| module Xcode | ||
| module Helper | ||
| module MacOS | ||
| module XcodeHelpers |
There was a problem hiding this comment.
IMO the word Helpers is just needless clutter in these modules. It adds no useful information at the call site.
There was a problem hiding this comment.
I agree - this module's scope is pretty big. Any name ideas?
There was a problem hiding this comment.
Like @ehanlon said, the best thing we could do is to extract appropriate methods to Chef::Provider.action_class. The size/scope of this module will also be reduced in the feature/xcode-idempotence branch.
There was a problem hiding this comment.
I like @brownmike's suggestion of using a module's static method - Xcode.installed? rather than xcode_installed.
There was a problem hiding this comment.
Since this is a pretty high-level library cookbook, I'd prefer we stick with idiomatic Chef and make contributions to Chef core if we think their current design modal can be improved.
There was a problem hiding this comment.
https://docs.chef.io/libraries.html uses the following custom namespace example for an ISP class:
ISP.vhosts.each do |vhost|
directory vhost[:documentroot] do
owner 'vhost[:uid]'
group 'vhost[:gid]'
mode '0755'
action :create
end
This change would put us more in line with the Chef docs, in my opinion.
libraries/xcode.rb
Outdated
| end | ||
| end | ||
|
|
||
| Chef::Recipe.include(MacOS::XcodeHelpers) |
There was a problem hiding this comment.
I personally don't like adding these methods directly to the global namespace of resources & recipes. It clutters the namespace, provides poor encapsulation, and forces the use of longer than needed method names in order to clarify intent. It's much cleaner if the call site can read something like Xcode.installed? rather than a global method xcode_installed?.
updated methods to simplify site calls in resource
This PR updates the name of the Xcode helper module and also automatically includes it in all recipes and resources.
I've also updated the Xcode test cookbook to run a suite specific to testing Xcode installs, and updated the default Xcode version node attribute to Xcode 9.1.