-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add Chef/RespondToResourceName and Chef/RespondToProvides cops #180
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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 ) ) ... ) | ||
PATTERN | ||
|
||
def autocorrect(node) | ||
lambda do |corrector| | ||
corrector.replace(node.loc.expression, node.children[1].source) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
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+ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't you target only the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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 |
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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.