Fixes #3309 - Support deep merging of hash and array structures in smart class parameters #1715
Conversation
orrabin
force-pushed
the
3636
branch
5 times, most recently
from
dc35bf8
to
f94842f
Compare
|
tests are failing on purpose for now |
| else | ||
| values[key_id][name][:value].deep_merge!(value[:value]) | ||
| values[key_id][name][:element] << value[:element] | ||
| # This was an attempt to handle element to only show the elements that won in the merge |
There was a problem hiding this comment.
@ohadlevy here is the problem I had with element
I have a problem predicting what the deep_merge will eliminate and finding which element had an impact on the final hash
orrabin
force-pushed
the
3636
branch
2 times, most recently
from
2075883
to
974880d
Compare
orrabin
changed the title
orrabin
force-pushed
the
3636
branch
7 times, most recently
from
4a35027
to
1eb2f68
Compare
orrabin
force-pushed
the
3636
branch
3 times, most recently
from
407b56b
to
4efa382
Compare
orrabin
force-pushed
the
3636
branch
2 times, most recently
from
44a97f5
to
6db0cda
Compare
|
[test] |
1 similar comment
|
[test] |
|
I'm a bit confused about bugs related to this PR.
Are all this issues going to be addressed in this PR? |
|
@mbacovsky #3636 is not addressed in this PR, this is only #3848 and #3309 which are the same bug but one is for arrays and one for hashes. |
|
@orrabin, with #3636 I see now, I was confused with the branch name. With the #3848 and #3309 I'm not sure though. From the bug descriptions it seems that the former is about matchers (hostgroup=[HG1,HG2]) and the later is about values and its deep merging (covering both hashes and array type values). So different issues unless I'm missing something. Makes sense? |
| @@ -35,6 +35,7 @@ def possible_value_orders | |||
|
|
|||
| def values_hash(options = {}) | |||
There was a problem hiding this comment.
This method is hard to read (not you fault) and you're adding more complexity. Therefore I'd consider doing some refactorings. First - you can extract values[key_id][name] into some variable so you don't have to repeat it on every line.
Then you can use hash with default values values = Hash.new {|h, k| h[k] = {}; values[:new_key][name] = triplet so you avoid hash[key_id] ||= {} lines.
Furthermore you don't have to store values_for_deep_merge in this complex hash format. I'd suggest using array OpenStruct or Struct instance for keeping value, index, element and element_name. Then calling hash_deep_merge that will generate values in desired hash structure. The benefit is that hash_deep_merge will be much easier to read. Ideally I'd use OpenStruct in whole values_hash method and convert the result into hash at the end (maybe some convert method).
Also I think you should compute values inside when "hash" branch, not looking at values_for_deep_merge outside of hash context in unless condition (this will also eliminate the need of values_for_deep_merge being global to the whole method).
Last but not least, I'd split this method so the actual evaluation is separated per type so we'd have methods like update_array_matcher, update_hash_matcher, update_generic_matcher which can be tested separately.
Let me know if I should elaborate any of previous refactorings. This file would deserves many other improvements but these are not related to the PR.
|
From user perspective I found one usability issue. When you set a overriden parameter to be an array or hash, Continue looking checkbox is not enabled so I have to save the parameter and then reopen the edit form to be able to check it. Same applies to Avoid duplicates which I can use after I save Continue looking checkbox value. This in combination with forgetting search filter after form submit is really annoying. |
| :override => true, :key_type => 'array', :merge_overrides => true, | ||
| :default_value => [], :path => "organization\nlocation", :avoid_duplicates => true, | ||
| :puppetclass => puppetclasses(:one)) | ||
| host = hosts(:one) |
There was a problem hiding this comment.
Could you please also use FactoryGirl to create a host? This will help us when we rebase the host factory PR.
|
@ares thanks for all the comments I will start refactoring :) |
|
Few more nitpicks based on our PR rules,
|
|
Just for the record, you're right, I had the older commit. JS now works just as I would expect 👍 . |
orrabin
force-pushed
the
3636
branch
2 times, most recently
from
f98c467
to
d126c1d
Compare
orrabin
changed the title
|
@ares I refactored |
isratrade
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Apart from few nitpicks the refactoring is very nice. |
|
@ares I fixed your comments, rebased and squashed commits :) |
…ures in smart class parameters
This relates to feature 3636 which states a couple of changes to the way
lookup_keysworkI'm creating a PR to get comments of this part of the change and to see that I'm in the right direction