Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Container Template: convert params to hashes #15587

Closed
wants to merge 1 commit into from
Closed

Container Template: convert params to hashes #15587

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/models/container_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,23 @@ def instantiate(params, project = nil)
:namespace => project
},
:objects => objects,
:parameters => params)
:parameters => params_to_hashes(params))
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.collect do |param|
{
:name => param.name,
:value => param.value,
:generate => param.generate,
:from => param.from,
:required => param.required
}
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method params_to_hashes returns an array? 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you convince our rubocop to not prefer collect? ;-)
Hey, did https://github.com/ManageIQ/ruby-style-guide evaporate ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh I'm too lazy to start that battle again, nevermind.

Yes, we torched it.

def process_template(client, template)
client.process_template(template)
rescue KubeException => e
Expand Down