theforeman / foreman_default_hostgroup

A plugin to set the default hostgroup when hosts are created.
GNU General Public License v3.0
11 stars 29 forks source link

Change request for default_hostgroup_managed_host_patch.rb for using all fact statements #18

Open ReneCamu opened 8 years ago

ReneCamu commented 8 years ago

Instead of using only using the first regex which fits, we changed the code so all statements needs to fit. default_hostgroup_managed_host_patch.rb.patch.txt

GregSutcliffe commented 8 years ago

Thanks for the patch!

This is a change in default behaviour - it's possible that others are already relying on the exisiting behaviour of matching the first regex, so I don't think we can merge this as is. I'm not against the idea, but I think it needs to be switchable behaviour based on a Setting. Would you be prepared to extend your patch for that?

If so, would you also be prepared to send it as a PR rather than a text patch? It's much easier to get our automated tests to run against PRs.

ReneCamu commented 8 years ago

Sorry I'm not so familiar with the PRs. I added two patches so it can be controlled through the settings.

default_hostgroup_managed_host_patch.rb.patch.txt default_hostgroup.rb.patch.txt

flyinbutrs commented 8 years ago

@ReneCamu

  1. Fork the repository to your account
  2. Clone your fork to your local machine
  3. Apply the changes locally
  4. Commit the changes locally
  5. Push the changes up to your fork on github
  6. On the github page for your repo, there should be a "Create Pull Request" option to create the PR.

Github Help Page

GregSutcliffe commented 8 years ago

Thanks @flyinbutrs - fyi @davividal already resubmitted this in #22 on @ReneCamu's behalf. I need to get around to looking at tests before merging it, but the new test structure should make this easier

flyinbutrs commented 8 years ago

Oops, I probably should have noticed that...

superfantasticawesome commented 7 years ago

This would be a truly great addition to default_hostgroup functionality as it would allow for combining facts for filtering on coordinates such as location/region/availability_zone/datacenter/subnet/etc.