Added AutomateWorkspace model #15817
Conversation
|
I still don't agree with this approach to the problem, but we can discuss as this moves forward. |
|
@mkanoor Do you have a design doc or picture for this. I'd like to understand all the moving parts before merging each individual gear. |
mkanoor
changed the title
|
Depends on Schema changes in ManageIQ/manageiq-schema#50 |
|
Merged schema in ManageIQ/manageiq-schema#50 |
app/models/automate_workspace.rb
Outdated
| raise ArgumentError, "No workspace or state_var specified for edit" | ||
| end | ||
|
|
||
| self[:output] = (output || {}).deep_merge(hash) |
There was a problem hiding this comment.
I believe it's preferable to use write_attribute over []= . @jrafanie @chrisarcand Do you know what's the preferable way to do this?
There was a problem hiding this comment.
Oh duh...the preferable way is to use super 😝
There was a problem hiding this comment.
Prefer output= on the parent via self.output = (output...., if that's what you meant by super, yeah 😄
app/models/automate_workspace.rb
Outdated
| end | ||
|
|
||
| self[:output] = (output || {}).deep_merge(hash) | ||
| save! |
There was a problem hiding this comment.
A setter should never call .save! directly. It's should be the caller's concern as they may choose to defer saving, or use update_attributes or whatever.
There was a problem hiding this comment.
That being said, can the setter just be the setter, and deal with the validation at validation time? Doing all this on a setter seems wrong.
There was a problem hiding this comment.
If you must have a method that sets and saves, then prefer creating a method like def merge_output!(hash) which is explicit in what it's doing
|
@Fryguy changed to merge_output! please review |
| @@ -0,0 +1,22 @@ | |||
| describe AutomateWorkspace do | |||
| context "#output=" do | |||
app/models/automate_workspace.rb
Outdated
| validates :user, :presence => true | ||
|
|
||
| def merge_output!(hash) | ||
| if hash['workspace'].blank? && hash['state_vars'].blank? |
There was a problem hiding this comment.
Should this be an ||? Based on the error message it should fail if either one is blank. Additionally, if state_vars => {} that would technically be blank as well, so maybe this should check for nil instead?
|
Checked commits mkanoor/manageiq@32db7e5~...bb02c3c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
Added AutomateWorkspace model which can be accessed from the REST API