Container Template: convert params to hashes #15587
Conversation
zakiva
force-pushed
the
convert_params_to_hashes
branch
from
23d5ee6 to
81f4395
Compare
|
@miq-bot add_label providers/containers, enhancement |
There was a problem hiding this comment.
👍
A bit of context: previously it was passing ActiveRecord objects to kubeclient (which somehow managed to turn it into json, but we shouldn't rely on this),
sending to openshift fields like id or created_on (which it seemed to ignore but we shouldn't rely on this).
app/models/container_template.rb
Outdated
| create_objects(processed_template['objects'], project) | ||
| @created_objects.each { |obj| obj[:miq_class] = MIQ_ENTITY_MAPPING[obj[:kind]] } | ||
| end | ||
|
|
||
| def params_to_hashes(params) | ||
| params.to_a.collect do |param| |
There was a problem hiding this comment.
Is .to_a necessary? If params is a single container template parameter, the call would fail. If it is an array, no need to call it.
There was a problem hiding this comment.
removed it, please note that caller anyway needs to pass an array or ActiveRecord collection (just template.container_template_parameters)
zakiva
force-pushed
the
convert_params_to_hashes
branch
from
81f4395 to
7b8e70e
Compare
|
Checked commit zakiva@7b8e70e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@gmcculloug Please review |
| } | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
This is a helper method, and should be added to the bottom of the file under a private so it doesn't get used publicly.
There's some duplication here, you could slice attributes to maintain a single list of the ones you want doing something like...
def params_to_hashes(params)
keys = %w(name value generate from required)
params.each_with_object([]) do |param, collection|
collection << param.attributes.slice(*keys)
end
end
There was a problem hiding this comment.
The method params_to_hashes returns an array? 😕
There was a problem hiding this comment.
That's what :parameters is expecting, yeah. Doesn't seem like the worst name; the more annoying part (that's beyond the scope of this change) is overloading the term params (used everywhere in a Rails app as a hash of parameters) with an array of ContainerTemplateParameters :)
There was a problem hiding this comment.
hashes (plural!) is widely used in refresh to mean array of hashes. Though yes, in ems_inv_to_hashes it means hash of arrays of hashes :-|
How about template_params_to_hashes ?
Why each_with_object([]), won't
params.collect do |param|
.param.attributes.slice("name", "value", "generate", "from", "required")
end
work?
@zakiva note this slice returns string keys and you had symbols.
There was a problem hiding this comment.
@cben Yes; I was going somewhere else in my brain before I realized params aren't just hashes themselves and are AR objects.
Prefer map over collect ;)
There was a problem hiding this comment.
Can you convince our rubocop to not prefer collect? ;-)
Hey, did https://github.com/ManageIQ/ruby-style-guide evaporate ?
There was a problem hiding this comment.
Ugh I'm too lazy to start that battle again, nevermind.
Yes, we torched it.
|
This pull request is not mergeable. Please rebase and repush. |
|
Closing in favor of ManageIQ/manageiq-providers-kubernetes#97 |
Converts the Container Template Parameters that are passed to
instantiatemethod to hashes before passing them to openshift for template processing. The new hashes include only relevant attributes for instantiation.