Conversation
| dirty = true | ||
| end | ||
| # if journal_device is nil, this will still work as expected | ||
| if node["ceph"]["osd_devices"][index]["journal"] != journal_device |
There was a problem hiding this comment.
Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
| end | ||
| node.set["ceph"]["osd_devices"][index]["status"] = "deployed" | ||
| node.set["ceph"]["osd_devices"][index]["journal"] = journal_device unless journal_device.nil? | ||
| if node["ceph"]["osd_devices"][index]["status"] != "deployed" |
There was a problem hiding this comment.
Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
|
|
||
| node.set["ceph"]["osd"]["use_ssd_for_journal"] = false unless has_ssds && has_hdds | ||
| use_ssd_for_journal = has_ssds && has_hdds | ||
| if node["ceph"]["osd"]["use_ssd_for_journal"] != use_ssd_for_journal |
There was a problem hiding this comment.
Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
| if !node["ceph"]["osd"]["journal_devices"].empty? | ||
| node.set["ceph"]["osd"]["use_ssd_for_journal"] = false | ||
| # explicit comparison because we don't want a condition that uses nil | ||
| if node["ceph"]["osd"]["use_ssd_for_journal"] != false |
There was a problem hiding this comment.
Metrics/BlockNesting: Avoid more than 3 levels of block nesting. (https://github.com/bbatsov/ruby-style-guide#three-is-the-number-thou-shalt-count)
|
|
||
| if is_crowbar? | ||
| node.set["ceph"]["osd_devices"] = [] if node["ceph"]["osd_devices"].nil? | ||
| dirty = false |
matelakat
left a comment
matelakat
left a comment
There was a problem hiding this comment.
@vuntz I see a pattern evolving here (and looking at the other changes as well) Can we make some sort of accessor/proxy/decorator that wraps the node, set attributes on the decorator, and it knows only to save the decorated object if any value changed?
|
Agreed with @matelakat here, I was thinking the same thing. Maybe cache the current attributes on the Nodeobject and override the save node to check if the current attributes have changed compared to the cached ones and call the parent save in that case? Not a ruby expert so this may not be possible or may incur a performance hit to have a cache of attributes? |
|
@matelakat @Itxaka let me think about it over lunch and see if I can come with something. |
|
So I'm torn. I can find ways to do something like suggested, but nothing that will end up looking like: node.set[:a][:b][:c] = foobar
node.saveThe most simple approach (from the caller perspective) would be something like: node.set_value(:a, :b, :c, foobar)
node.save_if_changedBut I kind of dislike the fact that we move away from what looks like standard chef code. I don't see a good way to simply override Anyone having feelings on this? |
|
It's evil, but it might be possible to monkey-patch |
|
And actually, if we did it right, probably we could just monkey-patch |
Rebase of PR crowbar#80 Only save the node when attribute(s) have changed Do not fetch monitor secret if we're the master
Rebase of PR crowbar#80 Only save the node when attribute(s) have changed Do not fetch monitor secret if we're the master
8 participants
Saving is not cheap, and most of the time, it's not needed (as no attribute changed, or because we will already save later).
(Here's it's not critical as the saves only occur on initial setup)