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

Jump to bottom

ansible refresh job_template -> playbook connection #14432

Merged
merged 2 commits into from
Apr 6, 2017
Merged

ansible refresh job_template -> playbook connection #14432

merged 2 commits into from
Apr 6, 2017

Conversation

durandom
Copy link
Member

@durandom durandom commented Mar 21, 2017 •
edited
Loading

attaches Playbook (ConfigurationScriptPayload) to JobTemplate (ConfigurationScript) via .parent during refresh

do what #14419 did in refresh

@durandom durandom mentioned this pull request Mar 21, 2017
Merged
@durandom durandom force-pushed the ansible_refresh_jt_playbook branch 3 times, most recently from bdc1d2c to 8deb24e Compare March 22, 2017 10:11
durandom
durandom commented Mar 22, 2017
View reviewed changes
app/models/manageiq/providers/ansible_tower/shared/inventory/parser/automation_manager.rb Outdated
@@ -32,6 +32,12 @@ def configuration_scripts
inventory_object.survey_spec = job_template.survey_spec_hash
inventory_object.variables = job_template.extra_vars_hash
inventory_object.inventory_root_group = persister.inventory_root_groups.lazy_find(job_template.inventory_id.to_s)
# inventory_object.configuration_script_source = persister.projects.lazy_find(job_template.project_id.to_s)
# project = persister.configuration_script_sources.lazy_find(job_template.project_id.to_s)
inventory_object.parent = persister.configuration_script_payloads.lazy_find(
Copy link
Member Author

Choose a reason for hiding this comment

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

@bzwei this is the right association, right?

job_template.parent = playbook / configuration_script.parent = configuration_script_payload

Copy link
Contributor

Choose a reason for hiding this comment

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

@durandom yes

Copy link
Contributor

Choose a reason for hiding this comment

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

is the parent using ancestry? If so, it might be best to do a custom saver like I have for AWS. Otherwise it's being saved ineffectively. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, its just a POA - plain old association

durandom
durandom commented Mar 22, 2017
View reviewed changes
app/models/manageiq/providers/ansible_tower/shared/inventory/parser/automation_manager.rb Outdated
@@ -32,6 +32,12 @@ def configuration_scripts
inventory_object.survey_spec = job_template.survey_spec_hash
inventory_object.variables = job_template.extra_vars_hash
inventory_object.inventory_root_group = persister.inventory_root_groups.lazy_find(job_template.inventory_id.to_s)
# inventory_object.configuration_script_source = persister.projects.lazy_find(job_template.project_id.to_s)
# project = persister.configuration_script_sources.lazy_find(job_template.project_id.to_s)
Copy link
Member Author

@durandom durandom Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jameswnl cant we store this info too?

job_template.project / configuration_script.configuration_script_source

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to? because job_template.playbook.project / configuration_script.configuration_script_payload.configuration_script_source is there to provide this info already

durandom
durandom commented Mar 22, 2017
View reviewed changes
app/models/manager_refresh/inventory_collection.rb
@@ -441,7 +441,7 @@ def find(manager_uuid)
when :local_db_find_missing_references
data_index[manager_uuid] || find_in_db(manager_uuid)
else
data_index[manager_uuid]
manager_uuid.kind_of?(Hash) ? find_by(manager_uuid) : data_index[manager_uuid]
Copy link
Member Author

Choose a reason for hiding this comment

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

@Ladas I had to do this to make adding lazy_find via a Hash (see the first and last commit in this PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, and then it just works?

I was thinking about adding a lazy_find_by, so maybe we should rather remove the find_by methods and just check for the Hash in find methods?

@durandom durandom force-pushed the ansible_refresh_jt_playbook branch from 8deb24e to 740b67f Compare March 22, 2017 10:53
@Ladas
Copy link
Contributor

Ladas commented Mar 22, 2017

looks good, the graph refresh change make sense, I'll to some consistency tweaks based on this later. Also having InventoryObject as part of the key will not work using the hash, so that is a thing to fix :-)

@durandom durandom force-pushed the ansible_refresh_jt_playbook branch from 740b67f to 7b87f44 Compare March 22, 2017 18:24
@durandom durandom force-pushed the ansible_refresh_jt_playbook branch from 7b87f44 to c9de871 Compare March 22, 2017 18:28
@durandom durandom changed the title [WIP] ansible refresh job_template -> playbook connection ansible refresh job_template -> playbook connection Mar 22, 2017
@durandom
Copy link
Member Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Mar 22, 2017
@miq-bot
Copy link
Member

miq-bot commented Mar 22, 2017

Checked commits durandom/manageiq@b6c481f~...c9de871 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks good. 🍪

@durandom
Copy link
Member Author

@blomquisg merge? @bzwei and @jameswnl gave their 👍 inline

@jameswnl jameswnl mentioned this pull request Mar 29, 2017
Merged
@durandom
Copy link
Member Author

@miq-bot add_label fine/yes

@durandom
Copy link
Member Author

@blomquisg merge?

jameswnl added a commit to jameswnl/manageiq that referenced this pull request Apr 3, 2017
@jameswnl
nullify dependents when destroying configuration_script_sources and c…
ff08d81 
…onfiguration_scripts

ManageIQ#14432
@blomquisg blomquisg merged commit e15393a into ManageIQ:master Apr 6, 2017
@blomquisg blomquisg added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 6, 2017
simaishi pushed a commit that referenced this pull request Apr 6, 2017
@blomquisg @simaishi
Merge pull request #14432 from durandom/ansible_refresh_jt_playbook
6034a20 
ansible refresh job_template -> playbook connection
(cherry picked from commit e15393a)
@simaishi
Copy link
Contributor

simaishi commented Apr 6, 2017

Fine backport details:

$ git log -1
commit 6034a201227275ddfed93a2d6c941471548711ef
Author: Greg Blomquist <[email protected]>
Date:   Thu Apr 6 16:05:17 2017 -0400

    Merge pull request #14432 from durandom/ansible_refresh_jt_playbook
    
    ansible refresh job_template -> playbook connection
    (cherry picked from commit e15393af24119d8f102f9be3500bed5694dea28f)

@durandom durandom deleted the ansible_refresh_jt_playbook branch April 7, 2017 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzwei bzwei bzwei left review comments

@jameswnl jameswnl jameswnl left review comments

@Ladas Ladas Ladas left review comments

Assignees
No one assigned
Labels
enhancement fine/backported providers/automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@durandom @Ladas @miq-bot @simaishi @jameswnl @bzwei @blomquisg @Fryguy @chessbyte