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

Add Chef/RespondToResourceName and Chef/RespondToProvides cops #180

Merged
merged 1 commit into from
Aug 14, 2019
Merged

Add Chef/RespondToResourceName and Chef/RespondToProvides cops #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions config/cookstyle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,22 @@ Chef/RespondToInMetadata:
Include:
- '**/metadata.rb'

Chef/RespondToResourceName:
Description: respond_to?(:resource_name) in resources is no longer necessary in Chef Infra Client 12.5+
Enabled: true
VersionAdded: '5.2.0'
Include:
- '**/resources/*.rb'
- '**/libraries/*.rb'

Chef/RespondToProvides:
Description: respond_to?(:provides) in resources is no longer necessary in Chef Infra Client 12+
Enabled: true
VersionAdded: '5.2.0'
Include:
- '**/providers/*.rb'
- '**/libraries/*.rb'

###############################
# Detecting code that breaks Chef
###############################
Expand Down
52 changes: 52 additions & 0 deletions lib/rubocop/cop/chef/modernize/respond_to_provides.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#
# Copyright:: 2019, Chef Software, Inc.
# Author:: Tim Smith (<tsmith@chef.io>)
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
module RuboCop
module Cop
module Chef
# It is not longer necessary respond_to?(:foo) in metadata. This was used to support new metadata
# methods in Chef 11 and early versions of Chef 12.
#
# @example
#
# # bad
# provides :foo if respond_to?(:provides)
#
# # good
# provides :foo
#
class RespondToProvides < Cop
MSG = 'respond_to?(:provides) in resources is no longer necessary in Chef Infra Client 12+'.freeze

def on_if(node)
if_respond_to_provides?(node) do
add_offense(node, location: :expression, message: MSG, severity: :refactor)
end
end

def_node_matcher :if_respond_to_provides?, <<~PATTERN
(if (send nil? :respond_to? ( :sym :provides ) ) ... )
PATTERN

def autocorrect(node)
lambda do |corrector|
corrector.replace(node.loc.expression, node.children[1].source)
end
end
end
end
end
end
53 changes: 53 additions & 0 deletions lib/rubocop/cop/chef/modernize/respond_to_resource_name.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#
# Copyright:: 2019, Chef Software, Inc.
# Author:: Tim Smith (<tsmith@chef.io>)
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
module RuboCop
module Cop
module Chef
# Chef 12.5 introduced the resource_name method for resources. Many cookbooks used
# respond_to?(:resource_name) to provide backwards compatibility with older chef-client
# releases. This backwards compatibility is no longer necessary.
#
# @example
#
# # bad
# resource_name :foo if respond_to?(:resource_name)
#
# # good
# resource_name :foo
#
class RespondToResourceName < Cop
MSG = 'respond_to?(:resource_name) in resources is no longer necessary in Chef Infra Client 12.5+'.freeze

def on_if(node)
if_respond_to_resource_name?(node) do
add_offense(node, location: :expression, message: MSG, severity: :refactor)
end
end

def_node_matcher :if_respond_to_resource_name?, <<~PATTERN
(if (send nil? :respond_to? ( :sym :resource_name ) ) ... )
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm guessing (if (send nil? ${:respond_to? ( :sym :resource_name )} ) ... ) (maybe without the {}'s again tho?) and then use the matcher instead of the expression for the location and the autocorrection (and then the autocorrect is simpler and deletes the entire match).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That particular match doesn't seem to work. The only thing you can do is match the whole bit since there's no negative match functionality.

PATTERN

def autocorrect(node)
lambda do |corrector|
corrector.replace(node.loc.expression, node.children[1].source)
end
end
end
end
end
end
47 changes: 47 additions & 0 deletions spec/rubocop/cop/chef/modernize/respond_to_provides_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#
# Copyright:: 2019, Chef Software, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

require 'spec_helper'

describe RuboCop::Cop::Chef::RespondToProvides, :config do
subject(:cop) { described_class.new(config) }

it 'registers an offense with a HWRP that uses respond_to? with provides' do
expect_violation(<<-RUBY)
class Chef
class Provider
class Icinga2Environment < Chef::Provider::LWRPBase
provides :icinga2_environment if respond_to?(:provides)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ respond_to?(:provides) in resources is no longer necessary in Chef Infra Client 12+
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you target only the if respond_to?(:provides) bit?

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'm not entirely sure how to do that to be honest.

Here's the ast of the above test:

(if
  (send nil :respond_to?
    (sym :provides))
  (send nil :provides
    (sym :icinga2_environment)) nil)

We need to match the if respond_to?(:provides), but you can't do a selector that doesn't select on the child node of (sym :icinga2_environment) as far as I know. That's why I matches the whole node and manipulated it in the autocorrect.

end
end
end
RUBY
end

it 'registers an offense with a LWRP that uses respond_to? with resource_name' do
expect_violation(<<-RUBY)
provides :icinga2_environment if respond_to?(:provides)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ respond_to?(:provides) in resources is no longer necessary in Chef Infra Client 12+
RUBY
end

it 'does not register an offense with a LWRP that does not use provides' do
expect_no_violations(<<-RUBY)
provides :icinga2_environment
RUBY
end
end
79 changes: 79 additions & 0 deletions spec/rubocop/cop/chef/modernize/respond_to_resource_name_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
#
# Copyright:: 2019, Chef Software, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

require 'spec_helper'

describe RuboCop::Cop::Chef::RespondToResourceName, :config do
subject(:cop) { described_class.new(config) }

it 'registers an offense with a HWRP that uses respond_to? with resource_name' do
expect_violation(<<-RUBY)
class Chef
class Resource
class Icinga2Apiuser < Chef::Resource
identity_attr :name

def initialize(name, run_context = nil)
super
@resource_name = :icinga2_apiuser if respond_to?(:resource_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ respond_to?(:resource_name) in resources is no longer necessary in Chef Infra Client 12.5+
@provides = :icinga2_apiuser
@provider = Chef::Provider::Icinga2Instance
@action = :create
@allowed_actions = [:create, :delete, :nothing]
@name = name
end
end
end
end
RUBY
end

it 'registers an offense with a HWRP that uses respond_to? with resource_name' do
expect_no_violations(<<-RUBY)
class Chef
class Resource
class Icinga2Apiuser < Chef::Resource
identity_attr :name

def initialize(name, run_context = nil)
super
@resource_name = :icinga2_apiuser
@provides = :icinga2_apiuser
@provider = Chef::Provider::Icinga2Instance
@action = :create
@allowed_actions = [:create, :delete, :nothing]
@name = name
end
end
end
end
RUBY
end

it 'registers an offense with a LWRP that uses respond_to? with resource_name' do
expect_violation(<<-RUBY)
resource_name :icinga2_apiuser if respond_to?(:resource_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ respond_to?(:resource_name) in resources is no longer necessary in Chef Infra Client 12.5+
RUBY
end

it 'does not register an offense with a LWRP that does not use resource_name' do
expect_no_violations(<<-RUBY)
resource_name :icinga2_apiuser
RUBY
end
end