ondemand icon indicating copy to clipboard operation
ondemand copied to clipboard

rails deep merge can't merge script.native attributes (arrays)

Open johrstrom opened this issue 4 years ago • 1 comments

This issue just came up in discourse and looking through the code I'm wondering if it's something that's more systemic.

The problem is this bc_num_slots is a smart attribute that sets some script.native attributes. Only when submit.yml.erb is read (and has some native attribute which is an array) and submitted it does a hash deep_merge (a rails helper for hashes) and I'm not sure you can deep_merge on arrays. The relevant merge is here

Of course if works for OSC because our native attributes are a hash. But for other adapters, native is an array. Which means folks cannot use bc_num_slots and script.native ( bc_num_slots is the only smart attribute that is native).

Here's a quick experiment that I did that illustrates this.

irb(main):004:0> require 'rails'
=> true
irb(main):001:0> script = {native: ['-n', '1'], a: 'b'}
=> {:native=>["-n", "1"], :a=>"b"}
irb(main):002:0> other = {native: ['-M', '512'], c: 'd'}
=> {:native=>["-M", "512"], :c=>"d"}
irb(main):005:0> script.deep_merge other
=> {:native=>["-M", "512"], :a=>"b", :c=>"d"}

┆Issue is synchronized with this Asana task by Unito

johrstrom avatar Mar 13 '20 17:03 johrstrom

Script#merge(other_script) could be added, and then this custom merge method could do a deep merge on the hashes like before but then set native to appending the original two together (if they are arrays).

Though extending the adapter to support the things that currently require native such as https://github.com/OSC/ood_core/issues/96 would also eliminate this problem, especially since bc_num_slots could be updated to use this new interface.

ericfranz avatar Mar 14 '20 01:03 ericfranz