theforeman / foreman_ansible_inventory

Foreman dynamic inventory script for ansible - Now merged into Ansible itself
GNU General Public License v3.0
70 stars 31 forks source link

Add group by Content View and Lifecycle Environment functionality #25

Closed nstrug closed 8 years ago

nstrug commented 8 years ago

Users of Satellite 6 need to be able to group by lifecycle environment and content views, functionality provided by Katello rather than Foreman. This PR provides that support.

dLobatog commented 8 years ago

@nstrug Thanks! Looks good to me - can you add information about this to the README? Ready to merge after that in my opinion

nstrug commented 8 years ago

README.md updated as requested.

dLobatog commented 8 years ago

Merged as 5eb6f77012718e8de8206f86a273e832b2b11ab9, thanks @nstrug !

agx commented 8 years ago

On Mon, Aug 08, 2016 at 10:13:19AM -0700, Nick Strugnell wrote:

Users of Satellite 6 need to be able to group by lifecycle environment and content views, functionality provided by Katello rather than Foreman. This PR provides that support. You can view, comment on, or merge this pull request online at:

https://github.com/theforeman/foreman_ansible_inventory/pull/25

-- Commit Summary --

  • added group by lifecycle_environment and contentview functionality
  • cleanups to katello-support stuff

Thanks, that looks useful!

Can you please squash these two commit?

It seems the second one fixes the first. Apart of that can we avoid duplicating the whole

for group in ... ...

loop e.g. via creating a list of groups and accessor functions:

a = [(lambda x: x, g) for g in ['hostgroup', 'environment', 'location', 'organization']]
b = [ (lambda x: x.get('content_facet_attributes', {}), g) for g in ['lifecycle_environment', 'content_view'] ]
for (acc, group) in a + b:
  ...

(with better names obviously).

nstrug commented 8 years ago

Any suggestions on how to squash the two commits together? Sorry, not a developer.

nstrug commented 8 years ago

Just noticed that the code changes were not actually merged, just the documentation change. Do you want me to submit a new PR?

dLobatog commented 8 years ago

Sorry, I'll cherrypick the original fix - my script failed to detect the 2 commits

dLobatog commented 8 years ago

1f12b6a1f66487ec2cea5789406755edc458b219 should contain it - feel free to submit the refactor in a different PR if you wish