Skip to content

Commit edeea19

Browse files
jonathannewmanRamesh7
jonathannewman
authored and
Ramesh7
committed
(maint) address rubocop complaints
1 parent 02beea5 commit edeea19

File tree

2 files changed

+79
-90
lines changed

2 files changed

+79
-90
lines changed

‎lib/puppet/type/node_group.rb

Lines changed: 43 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
end
3636
newproperty(:rule, array_matching: :all) do
3737
desc 'Match conditions for this group'
38+
# rubocop:disable Metrics/CyclomaticComplexity
39+
# rubocop:disable Metrics/PerceivedComplexity
3840
def should
3941
case @resource[:purge_behavior]
4042
# only do something special when we are asked to merge, otherwise, fall back to the default
@@ -57,67 +59,55 @@ def should
5759
# pinned nodes are in the form ['=', 'name', <hostname>]
5860
apinned = []
5961
a_without_pinned = a
60-
if (a[0] == 'or')
61-
apinned = a.select {|item| (item[0] == '=') && (item[1] == 'name')}
62+
if a[0] == 'or'
63+
apinned = a.select { |item| (item[0] == '=') && (item[1] == 'name') }
6264
a_without_pinned = a.select { |item| (item[0] != '=') || (item[1] != 'name') }
6365
end
6466
bpinned = []
6567
b_without_pinned = b
6668
merged = []
67-
if (a == [""])
68-
# ensure rules are unique at the top level and remove any empty rule sets
69-
return b.uniq.select { |item| (item != ['or'] && item != ['and'] )}
70-
elsif (b == [""])
71-
# ensure rules are unique at the top level and remove any empty rule sets
72-
return a.uniq.select { |item| (item != ['or'] && item != ['and'] )}
73-
end
7469

75-
if (b[0] == 'or')
76-
bpinned = b.select {|item| (item[0] == '=') && (item[1] == 'name')}
70+
(return b.uniq.select { |item| (item != ['or'] && item != ['and']) } if a == [''])
71+
(return a.uniq.select { |item| (item != ['or'] && item != ['and']) } if b == [''])
72+
73+
if b[0] == 'or'
74+
bpinned = b.select { |item| (item[0] == '=') && (item[1] == 'name') }
7775
b_without_pinned = b.select { |item| (item[0] != '=') || (item[1] != 'name') }
7876
end
7977

80-
if (((a[0] == 'and') || (a[0] == 'or')) && a[0] == b[0])
81-
# if a and b start with the same 'and' or 'or' clause, we can just combine them
82-
if (a[0] == 'or')
83-
merged = (['or'] + a_without_pinned.drop(1) + b_without_pinned.drop(1) + apinned + bpinned).uniq
84-
else
85-
# must both be 'and' clauses
86-
if (apinned.length > 0 || bpinned.length > 0)
87-
# we have pinned nodes
88-
merged = (['or'] + [a_without_pinned + b_without_pinned.drop(1)] + apinned + bpinned).uniq
89-
else
90-
# no pinned nodes and one top level 'and' clause, just combine them.
91-
merged = a_without_pinned + b_without_pinned.drop(1)
92-
end
93-
end
94-
else
95-
# first clause of a and b aren't equal
96-
# a first clause is one of and/or/not/operator
97-
# b first clause is one of and/or/not/operator
98-
# if a starts with `and` and b starts with `or`, create a top level `or` clause, nest a under it and append the rest of b
99-
if (a_without_pinned[0] == 'and' && b_without_pinned[0] == 'or')
100-
# special case of a only having one subclause
101-
if (a_without_pinned.length == 2)
102-
merged = (['or'] + a_without_pinned[1] + b_without_pinned.drop(1) + apinned + bpinned)
103-
else
104-
merged = (['or'] + [a_without_pinned] + b_without_pinned.drop(1) + apinned + bpinned)
105-
end
106-
else
107-
if (a_without_pinned[0] == 'or')
108-
merged = (a_without_pinned + [b_without_pinned] + apinned + bpinned).uniq
109-
else
110-
# if b starts with 'or', we want to be sure to drop that.
111-
if (b_without_pinned[0] == 'or')
112-
merged = (['or'] + [a_without_pinned] + b_without_pinned.drop(1) + apinned + bpinned)
113-
else
114-
merged = (['or'] + [a_without_pinned] + [b_without_pinned] + apinned + bpinned)
115-
end
116-
end
117-
end
118-
end
78+
merged = if ((a[0] == 'and') || (a[0] == 'or')) && a[0] == b[0]
79+
# if a and b start with the same 'and' or 'or' clause, we can just combine them
80+
if a[0] == 'or'
81+
(['or'] + a_without_pinned.drop(1) + b_without_pinned.drop(1) + apinned + bpinned).uniq
82+
elsif apinned.length.positive? || bpinned.length.positive?
83+
# must both be 'and' clauses
84+
(['or'] + [a_without_pinned + b_without_pinned.drop(1)] + apinned + bpinned).uniq
85+
# we have pinned nodes
86+
else
87+
# no pinned nodes and one top level 'and' clause, just combine them.
88+
a_without_pinned + b_without_pinned.drop(1)
89+
end
90+
elsif a_without_pinned[0] == 'and' && b_without_pinned[0] == 'or'
91+
# first clause of a and b aren't equal
92+
# a first clause is one of and/or/not/operator
93+
# b first clause is one of and/or/not/operator
94+
# if a starts with `and` and b starts with `or`, create a top level `or` clause, nest a under it and append the rest of b
95+
if a_without_pinned.length == 2
96+
(['or'] + a_without_pinned[1] + b_without_pinned.drop(1) + apinned + bpinned)
97+
else
98+
(['or'] + [a_without_pinned] + b_without_pinned.drop(1) + apinned + bpinned)
99+
end
100+
# special case of a only having one subclause
101+
elsif a_without_pinned[0] == 'or'
102+
(a_without_pinned + [b_without_pinned] + apinned + bpinned).uniq
103+
elsif b_without_pinned[0] == 'or'
104+
# if b starts with 'or', we want to be sure to drop that.
105+
(['or'] + [a_without_pinned] + b_without_pinned.drop(1) + apinned + bpinned)
106+
else
107+
(['or'] + [a_without_pinned] + [b_without_pinned] + apinned + bpinned)
108+
end
119109
# ensure rules are unique at the top level and remove any empty rule sets
120-
merged.uniq.select { |item| (item != ['or'] && item != ['and'] )}
110+
merged.uniq.select { |item| (item != ['or'] && item != ['and']) }
121111
else
122112
super
123113
end
@@ -127,6 +117,8 @@ def insync?(is)
127117
is == should
128118
end
129119
end
120+
# rubocop:enable Metrics/CyclomaticComplexity
121+
# rubocop:enable Metrics/PerceivedComplexity
130122
newproperty(:environment) do
131123
desc 'Environment for this group'
132124
defaultto :production

‎spec/unit/puppet/type/node_group_spec.rb

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
require 'spec_helper'
22

3+
# rubocop:disable Metrics/BlockLength
34
describe Puppet::Type.type(:node_group) do
45
it 'allows agent-specified environment' do
56
expect {
@@ -73,8 +74,6 @@
7374
end
7475

7576
describe 'purge_behavior' do
76-
77-
7877
let(:and_rule) do
7978
['and',
8079
['~', ['fact', 'fact1'], 'value1'],
@@ -105,14 +104,14 @@
105104
}
106105
end
107106
let(:group_with_merged_and_rule) do
108-
["or",
109-
["and",
110-
["~", ["fact", "fact1"], "value1"],
111-
["~", ["fact", "fact2"], "value2"]],
112-
["~", ["fact", "fact1"], "value1"],
113-
["~", ["fact", "fact2"], "value2"]]
114-
end
115-
107+
['or',
108+
['and',
109+
['~', ['fact', 'fact1'], 'value1'],
110+
['~', ['fact', 'fact2'], 'value2']],
111+
['~', ['fact', 'fact1'], 'value1'],
112+
['~', ['fact', 'fact2'], 'value2']]
113+
end
114+
116115
it 'replaces by default' do
117116
rsrc = described_class.new(group_with_rule)
118117
# this is simulated data from the classifier service
@@ -163,20 +162,20 @@
163162
# to the end
164163
# also the merging optimizes the pinned nodes to remove redundants.
165164
let(:merged_pinned_nodes_and_rule) do
166-
["or",
167-
["and",
168-
["~", ["fact", "fact1"], "value1"],
169-
["~", ["fact", "fact2"], "value2"]],
170-
["=", "name", "value1"],
171-
["=", "name", "value2"]]
165+
['or',
166+
['and',
167+
['~', ['fact', 'fact1'], 'value1'],
168+
['~', ['fact', 'fact2'], 'value2']],
169+
['=', 'name', 'value1'],
170+
['=', 'name', 'value2']]
172171
end
173172

174173
let(:merged_pinned_nodes_or_rule) do
175-
["or",
176-
["~", ["fact", "fact1"], "value1"],
177-
["~", ["fact", "fact2"], "value2"],
178-
["=", "name", "value1"],
179-
["=", "name", "value2"]]
174+
['or',
175+
['~', ['fact', 'fact1'], 'value1'],
176+
['~', ['fact', 'fact2'], 'value2'],
177+
['=', 'name', 'value1'],
178+
['=', 'name', 'value2']]
180179
end
181180

182181
it 'matches rule exactly by default' do
@@ -199,9 +198,9 @@
199198

200199
it 'merges pinned node correctly when purge behaviour set to :none' do
201200
rsrc = described_class.new(group_with_pinned_nodes.merge(purge_behavior: 'none'))
202-
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(["=", "name", "value2"])
201+
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(['=', 'name', 'value2'])
203202
# redundant node pinning is removed
204-
expect(rsrc.property(:rule).should).to eq ["or", ["=", "name", "value2"], ["=", "name", "value1"]]
203+
expect(rsrc.property(:rule).should).to eq ['or', ['=', 'name', 'value2'], ['=', 'name', 'value1']]
205204
end
206205

207206
it 'replaces rule when purge behaviour set to :all' do
@@ -215,7 +214,6 @@
215214
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(and_rule)
216215
expect(rsrc.property(:rule).should).to eq group_with_pinned_nodes[:rule]
217216
end
218-
219217
end
220218

221219
describe 'group with top-level and clause' do
@@ -236,17 +234,17 @@
236234
end
237235

238236
let(:rule_combined_and_clauses) do
239-
["and",
240-
["~", ["fact", "fact1"], "value1"],
241-
["~", ["fact", "fact2"], "value2"],
242-
["~", ["fact", "pe_server_version"], ".+"]]
237+
['and',
238+
['~', ['fact', 'fact1'], 'value1'],
239+
['~', ['fact', 'fact2'], 'value2'],
240+
['~', ['fact', 'pe_server_version'], '.+']]
243241
end
244242

245243
let(:rule_combined_or_clauses) do
246-
["or",
247-
["~", ["fact", "fact1"], "value1"],
248-
["~", ["fact", "fact2"], "value2"],
249-
["and", ["~", ["fact", "pe_server_version"], ".+"]]]
244+
['or',
245+
['~', ['fact', 'fact1'], 'value1'],
246+
['~', ['fact', 'fact2'], 'value2'],
247+
['and', ['~', ['fact', 'pe_server_version'], '.+']]]
250248
end
251249

252250
it 'replaces rule exactly by default' do
@@ -269,9 +267,9 @@
269267

270268
it 'merges pinned node correctly when purge behaviour set to :none' do
271269
rsrc = described_class.new(group_with_and_clause.merge(purge_behavior: 'none'))
272-
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(["=", "name", "value2"])
270+
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(['=', 'name', 'value2'])
273271
# redundant node pinning is removed
274-
expect(rsrc.property(:rule).should).to eq ["or", ["=", "name", "value2"], ["and", ["~", ["fact", "pe_server_version"], ".+"]]]
272+
expect(rsrc.property(:rule).should).to eq ['or', ['=', 'name', 'value2'], ['and', ['~', ['fact', 'pe_server_version'], '.+']]]
275273
end
276274

277275
it 'replaces rule when purge behaviour set to :all' do
@@ -285,7 +283,6 @@
285283
allow(rsrc.property(:rule)).to receive(:retrieve).and_return(and_rule)
286284
expect(rsrc.property(:rule).should).to eq group_with_and_clause[:rule]
287285
end
288-
289286
end
290287

291288
describe 'resource hash' do
@@ -305,7 +302,7 @@
305302
},
306303
}
307304
end
308-
305+
309306
let(:existing_data) do
310307
{ 'data::class1' => { 'param1' => 'existing',
311308
'param3' => 'existing' },
@@ -321,7 +318,7 @@
321318
'data::class3' => { 'param1' => 'existing',
322319
'param2' => 'existing' } }
323320
end
324-
321+
325322
let(:existing_classes) do
326323
{ 'classes::class1' => { 'param1' => 'existing',
327324
'param3' => 'existing' },
@@ -335,7 +332,7 @@
335332
'classes::class3' => { 'param1' => 'existing',
336333
'param2' => 'existing' } }
337334
end
338-
335+
339336
it 'matches classes and data exactly by default' do
340337
rsrc = described_class.new(resource_hash)
341338
allow(rsrc.property(:data)).to receive(:retrieve).and_return(existing_data)
@@ -368,7 +365,6 @@
368365
expect(rsrc.property(:classes).should).to eq(resource_hash[:classes])
369366
end
370367
end
371-
372368
end
373369

374370
describe '.insync? for data, classes' do
@@ -414,3 +410,4 @@
414410
end
415411
end
416412
end
413+
# rubocop:enable Metrics/BlockLength

0 commit comments

Comments
 (0)