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

Implement spotlight resource, metadata_util library, and add tests/documentation #34

Merged
merged 30 commits into from
Jan 28, 2018

Conversation

jazaval
Copy link
Contributor

@jazaval jazaval commented Jan 26, 2018

This PR implements a spotlight resource, a metadata_util library, adds unit tests, a recipe to the test cookbook with smoke, and documentation.

spotlight

Use the spotlight resource to manage the metadata indexing state for disk volumes. This
will primarily affect the ability to search volume contents with the macOS Spotlight feature.
Under the hood, a spotlight resource executes the mdutil command in the metadata_util
library.

Learn more about Spotlight.

Learn more about the mdutil command.

Syntax

The most basic usage of a spotlight resource block declares a disk volume as the name property
to enable metadata indexing:

spotlight '/'

The full syntax for all of the properties available to the spotlight resource is:

spotlight 'volume name' do
  volume                      String # defaults to 'volume name' if not specified
  indexed                     TrueClass, FalseClass # defaults to True if not specified
  searchable                  TrueClass, FalseClass # defaults to True if not specified
end

Actions

This resource has the following actions:

:set

       Set the metadata indexing state declared by the indexed property. This is the only, and default, action.

Properties

volume

      Ruby Type: String

       The name of the disk volume to manage.

indexed

      Ruby type: TrueClass, FalseClass

       Whether or not the desired state of the named disk volume is to be indexed.

searchable

      Ruby type: TrueClass, FalseClass

       Disables Spotlight searching if the index has already been created for the volume. Only applicable if the indexed property is set to false.

Examples

spotlight '/' # enables indexing on the boot volume

spotlight 'test_disk1' do # disables indexing on 'test_disk1'
  indexed false
end

spotlight 'enable indexing on TDD2' do
  volume 'TDD2'
  indexed true
end

spotlight 'disable indexing and prevent searching index on TDD-ROM' do
  volume 'TDD-ROM'
  indexed false
  searchable false
end

Copy link
Collaborator

@americanhanko americanhanko left a comment

Choose a reason for hiding this comment

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

I've got a couple of nitpicks, but this is a super useful resource to add to our arsenal. Nice work!

```ruby
spotlight 'volume name' do
volume String # defaults to 'volume name' if not specified
indexed TrueClass, FalseClass # defaults to TrueClass if not specified
Copy link
Collaborator

Choose a reason for hiding this comment

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

indexing feels a little vague and doesn't immediately strike me as a boolean property. I think making it explicit might make it clearer, especially if it has a default property. How about something like indexing_disabled and have it default to false?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is fine. In my head, I usually read these properties as is itindexed?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@americanhanko indexing isn't the property name - it's indexed.

Copy link
Collaborator

@americanhanko americanhanko Jan 27, 2018

Choose a reason for hiding this comment

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

Oops! Thanks for that correction.

What I'm getting at is that by naming the property indexed, it's as if there is a question mark there: indexed? -- that would make sense given the Ruby convention for methods that return a boolean value. However, since we can't use a question mark in a property name, I thought it would be better to make it explicit: indexing_enabled with a default value of false (or vice versa).

Conversely, it turns out boolean properties across Chef (core, community or otherwise) resources do not share a consistent naming strategy, and in fact follow a similar pattern of "leaving out the question mark". Either way works for me 👍

Edit: for another argument against my original qualm, the Ruby convention is to indicate the return value of a method, whereas in a Chef::Resource, it's part of the constructor and you can see it in plain sight: indexed false

spotlight 'volume name' do
volume String # defaults to 'volume name' if not specified
indexed TrueClass, FalseClass # defaults to TrueClass if not specified
allow_search TrueClass, FalseClass # defaults to TrueClass if not specified
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar here: I think it might be better to have this be searching_disabled and match the same sort of format for the boolean property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this to searchable for clarity.


def volume_current_state(volume)
shell_out("/usr/bin/mdutil -s #{volume.shellescape}")
.stdout.split(':')[1].strip
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nitpick, but we do want to be careful of method chaining; as well as taking advantage of shell_out's ability to take any number of arguments for its constructor (doing so may allow use to avoid needing shellescape?)

def volume_current_state(volume)
  status_output = shell_out('/usr/bin/mdutil', '-s', volume.shellescape).stdout.strip # handle the String first
  status_output.split(':')[1] # and the Array second
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this method to let shell_out handle escape characters. Who knew? ;-)

end

action :set do
execute "turn Spotlight indexing #{state} for #{target_volume}" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

"turn Spotlight indexing #{state} for #{target_volume}"

Beautiful!

volume_path(new_resource.volume)
end

def volume_path(volume)
Copy link
Collaborator

@americanhanko americanhanko Jan 27, 2018

Choose a reason for hiding this comment

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

We can use a ternary operator here, as well as using ::File.join to avoid the string interpolation.

def volume_path(volume)
  volume == '/' ? volume : ::File.join('/Volumes', volume)
end

Copy link

@brownmike brownmike left a comment

Choose a reason for hiding this comment

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

Great looking PR, @jazaval

Properties
----------

`volume`

Choose a reason for hiding this comment

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

IMO I think this data would look more readable in plaintext with the use of a markdown table while still being quite digestible when rendered.

@jazaval jazaval merged commit bcc4fe5 into develop Jan 28, 2018
@jazaval jazaval deleted the feature/spotlight branch January 28, 2018 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants