Skip to content
This repository has been archived by the owner on Oct 17, 2019. It is now read-only.

Prefer hostgroup title over hostgroup name #39

Closed
wants to merge 1 commit into from
Closed

Conversation

agx
Copy link
Member

@agx agx commented Oct 12, 2016

Foreman changed behviour in 1.12: the hostgroup_name does no longer
contain the parent hostgroups like " a / b / c " but the title does so
use this for the building the ansible groups. Fall back to
hostgroup_name for older versions.

Closes #27

@agx agx force-pushed the hostgroup-title branch from adf2314 to b173138 Compare October 12, 2016 18:31
@agx agx changed the title Use hostgroup title instead of name Prefer hostgroup title over hostgroup name Oct 12, 2016
@smnmtzgr
Copy link

I think your change wouldn`t be functional as the "group"-variable is missing and so line 262 will fail.

Foreman changed behviour in 1.12: the hostgroup_name does no longer
contain the parent hostgroups like " a / b / c " but the title does so
use this for the building the ansible groups. Fall back to
hostgroup_name for older versions.

Closes #27
@agx agx force-pushed the hostgroup-title branch from b173138 to 47b0b40 Compare October 13, 2016 06:52
@agx
Copy link
Member Author

agx commented Oct 13, 2016

On Wed, Oct 12, 2016 at 11:48:43PM -0700, Simon wrote:

I think your change wouldn`t be functional as the "group"-variable is missing and so line 262 will fail.

You're right. No idea why this didn't trigger in my tests. I've updated
the PR but I'm fine with yours if you unbreak older Foreman by handling
_name as well (See 260 in my PR).

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#39 (comment)

@smnmtzgr
Copy link

done, please check again.

@agx
Copy link
Member Author

agx commented Oct 13, 2016

On Thu, Oct 13, 2016 at 04:54:45AM -0700, Simon wrote:

done, please check again.

Looks good. Can you squash the 4 commits into one with a nice commit
message so we get a feature based histoy instead of a time based one?

I'm happy to merge it then.

@smnmtzgr
Copy link

squashed into one commit. Please check again.

@agx
Copy link
Member Author

agx commented Oct 14, 2016

Not needed due to #38

@agx agx closed this Oct 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants