Fix Settings issue where Arrays would merge instead of overwrite #7670
Conversation
The ability to override Arrays has been made into a PR upstream at rubyconfig/config#137 and danielsdeleo/deep_merge#21 . However, they are not yet merged. When they are merged and released, we can switch back to a released version. Supercedes ManageIQ#7656 Paired on this with @carbonin
|
Checked commit Fryguy@5e0215c with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1 Gemfile
|
|
<github_pr_commenter_batch />Some comments on commit Fryguy@5e0215c |
| end | ||
|
|
||
| it "with database changes on an Array" do | ||
| server.settings_changes.create!(:key => "/log/collection/current/pattern", :value => ["*.log"]) |
There was a problem hiding this comment.
If I understand this correctly, these tests are testing this change only if the settings.yml originally has an array in /log/collection/current/pattern. Does it make sense to not depend on the current values in the settings.yml by injecting a desired value for it?
There was a problem hiding this comment.
Note, I ask this because I didn't originally realize what this PR was doing since I couldn't see that the existing value was an array we want to replace instead of merge.
There was a problem hiding this comment.
Yeah, I agree injecting a value would be better, but I'm not sure how to do that. I think we would have to add an additional config source for specs. @Fryguy ?
There was a problem hiding this comment.
So, stubbing settings is an incredible pain in the neck, particularly because we build new Settings constants on the fly when things change. I have a helper method that stubs the Settings const as well as for_resource, but then that, of course, bypasses all the database stuff, which these methods are trying to test directly
Basically, I'm not really sure how to test the Settings machinery by stubbing the Settings machinery 😝
There was a problem hiding this comment.
Maybe you can change the the example's description in a followup PR, something like "with database changes for an Array field from a template source"
The ability to override Arrays has been made into a PR upstream at
rubyconfig/config#137 and
danielsdeleo/deep_merge#21 . However, they
are not yet merged. When they are merged and released, we can
switch back to a released version.
Supercedes #7656
Paired on this with @carbonin
@jrafanie Please review.