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
Fixes #27937 - API for Ansible Inventory template scheduling #299
Conversation
163075a
to
f7e803c
Compare
We would probably also need the below fields ( or decide which ones we need from the below response) from the earlier setup:
|
@ofedoren : I am not sure if I am using the required param correctly, but with something like this: {'required':'all'} I get an error. Do you have an example curl with what the hash expects? |
required = input('required').split(',').map { |r| r.strip.downcase } | ||
inventory_data.merge!('Organization': host.organization) if (['all', 'organization'] & required).any? | ||
inventory_data.merge!('Location': host.location) if (['all', 'location'] & required).any? | ||
inventory_data.merge!('Hostgroup': host.hostgroup.name) if (['all', 'hostgroup'] & required).any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inventory_data.merge!('Hostgroup': host.hostgroup.name) if (['all', 'hostgroup'] & required).any? | |
inventory_data.merge!('Hostgroup': host.hostgroup.name) if (['all', 'hostgroup'] & required).any? && host.hostgroup |
to avoid nil reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So based on the new fields we want to add, I tried this:
['architecture_id','architecture_name'].each do |field| inventory_data.merge!(field=>host[field])
nor can I do something like inventory_data.merge!(field=>host.send(field))
It seems the Host::Managed::Jail
does not like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments, please let me know if I should provide more details
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
- name: required | ||
required: false | ||
input_type: user | ||
description: Comma-separated values that should be returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nicer user experience if we define separate input for every optional part, it would have restricted values yes no and would be required. That in UI should generate select boxes to chose from yes/no.
We could also use this chance to add default values to inputs (that would be PR to Foreman core)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: theforeman/foreman#7050
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
Below is the list of fields available on the host model so they can be pulled into the report: |
f7e803c
to
b0ee88e
Compare
The third commit requires Katello/katello#8356. |
The fourth commit requires theforeman/foreman#7052. |
@sjha4, that's quite a big list and I'm not sure if we should add a separate template input for each field. @ares, how can we choose what should be in the report (or at least as template inputs) and what should be not? |
@ofedoren : Right..It's >70 fields in the intial API call I was testng with. I am not sure we need all the details on the report. |
@sjha4, thanks for your input! I've added Content Facet Attributes to the template. |
You can use https://projects.theforeman.org/issues/27937 for tracking this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far, leaving few comments
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
61bef82
to
eb0d67e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for review! To simplify (I hope for testing purposes as well) I squashed the commits to one and to correspond with the issue.
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
app/views/foreman_ansible/job_templates/ansible_-_inventory.erb
Outdated
Show resolved
Hide resolved
looks well now, safe mode PRs seems to be in, this is now blocked on default values, description and format PRs in core |
eb0d67e
to
a037cc3
Compare
The template has been updated:
|
a037cc3
to
7588d58
Compare
7588d58
to
3e077fb
Compare
please rebase |
3e077fb
to
2060412
Compare
@mmoll, rebased. |
now it fails on rubocop |
2060412
to
9bb59b4
Compare
@mmoll, thanks :) fixed. |
almost there, permission missing :) |
9bb59b4
to
ae55223
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably resuse existing core permission for generating reports
lib/foreman_ansible/register.rb
Outdated
@@ -82,7 +82,7 @@ | |||
:edit_ansible_variables, :destroy_ansible_variables] | |||
|
|||
role 'Ansible Tower Inventory Reader', | |||
[:view_hosts, :view_hostgroups, :view_facts], | |||
[:view_hosts, :view_hostgroups, :view_facts, :view_schedule], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need separate view_schedule permission? I think this should be the same permission as for genertimg regular report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reused generate_report_tempaltes
permission if it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's exactly what I had in mind
ae55223
to
74c6206
Compare
74c6206
to
7267312
Compare
[test foreman_ansible] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, ack pending jenkins
Thanks @ofedoren, merging! |
Requires theforeman/foreman#6875.
To use the endpoint there should exist the
Ansible Inventory
report template (part of this PR)