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.

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

Include resource_actions and picture in service_template copy #18973

Merged

Conversation

gmcculloug
Copy link
Member

@gmcculloug gmcculloug commented Jul 13, 2019

Added missing associations to the service_template #template_copy method and marked existing helper methods as private.

Recommend reviewing commit-by-commit.

  • First commit only moves the existing tests into a separate files (matching the separate file the template_copy code lives in)
  • Second commit copies the missing resources
  • Third commit moves the save! method so the newly create template is returned from the call, instead of true from the save! call and makes helper methods private.
  • Final commit adds tests for the new resources.

Fixes #18949
Closes #18450

Links

Found while working on UI changes ManageIQ/manageiq-ui-classic#5667
Extends work from #18464

Steps for Testing/QA

After copying a service_template validate that it contains the same Automate entry-points, picture and additional_tenants on the summary screen as the original service_template.

cc @d-m-u

@gmcculloug
Copy link
Member Author

@tinaafitz I did not make the change here but I'm thinking with the change to Ansible runner we can remove the restriction on ServiceTemplateAnsiblePlaybook as we do not have to create the JobTemplate anymore. Thoughts? cc @lfu

@gmcculloug gmcculloug force-pushed the service_template_copy_actions_picture branch from cab68d0 to eb40d1b Compare July 13, 2019 19:30
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova left a comment

Choose a reason for hiding this comment

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

Tested with UI PR. LGTM 👍

Thanks for fixing it :)

@@ -7,6 +7,9 @@ def template_copy(new_name = "Copy of " + name + Time.zone.now.to_s)
dup.tap do |template|
template.update_attributes(:name => new_name, :display => false)
service_resources.each { |service_resource| resource_copy(service_resource, template) }
resource_actions.each { |resource_action| resource_action_copy(resource_action, template) }
picture_copy(template) if picture.present?
Copy link
Member

Choose a reason for hiding this comment

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

Is the .present? necessary? picture should be a Picture instance or nil, right?

]

new_template = @st1.template_copy
expect(new_template.resource_actions.collect(&:action)).to eq(%w[Provision Retire])
Copy link
Member

Choose a reason for hiding this comment

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

Would expect(new_template.resource_actions.pluck(:action)).to match_array(%w[Provision Retire]) be faster and more reliable?

@lfu
Copy link
Member

lfu commented Jul 15, 2019

@tinaafitz I did not make the change here but I'm thinking with the change to Ansible runner we can remove the restriction on ServiceTemplateAnsiblePlaybook as we do not have to create the JobTemplate anymore. Thoughts? cc @lfu

The cleanup of JobTemplate with Ansible runner is not going to happen at the moment. cc @carbonin

@carbonin
Copy link
Member

@lfu I said that it's not likely that I will get to cleaning up the job template logic. If someone else wants to take it on, that's fine with me.

@gmcculloug gmcculloug force-pushed the service_template_copy_actions_picture branch from eb40d1b to 6afcdac Compare July 16, 2019 02:42
@gmcculloug gmcculloug force-pushed the service_template_copy_actions_picture branch from 6afcdac to 7e176fa Compare July 16, 2019 02:44
@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2019

Checked commits gmcculloug/manageiq@4c3f832~...7e176fa with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

spec/models/service_template/copy_spec.rb

@bdunne bdunne merged commit bf53b44 into ManageIQ:master Jul 16, 2019
@bdunne bdunne added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 16, 2019
@gmcculloug gmcculloug deleted the service_template_copy_actions_picture branch January 1, 2020 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing required data in copied ServiceTemplate [RFE] Service Item Copy
6 participants