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.

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

Wrap role identifiers in quotes #47

Merged
merged 3 commits into from
May 25, 2016

Conversation

jimbocoder
Copy link
Contributor

Instead of using complicated normalization heuristics

Rather than have a bunch of rules about valid role names,
the dot format allows quoting identifiers, so this is much easier
and should fix related issues including nickjj#46
This allows running `ansigenome export` somewhere other than ROLES_PATH.
This fixes nickjj#21 "more correctly".
@nickjj
Copy link
Owner

nickjj commented May 25, 2016

Looks promising, thanks a lot. Can you fix the flake8 issues reported in https://travis-ci.org/nickjj/ansigenome/builds/132960127?

@jimbocoder
Copy link
Contributor Author

Done!

@nickjj nickjj merged commit fcc4f7a into nickjj:master May 25, 2016
@nickjj
Copy link
Owner

nickjj commented May 25, 2016

And merged, thanks.

@ypid
Copy link
Contributor

ypid commented Jun 18, 2016

86a62d4 breaks the role name and with this also the generated links!

I use Ansigenome like this ansigenome gendoc --format=md --limit "$(basename "$PWD")" .. from inside a role. Before 86a62d4, the role name was just ${rolename}, now it is ${user}.${rolename}.

@jimbocoder
Copy link
Contributor Author

@ypid I haven't yet looked closely at this, but I have some thoughts. Do you see a simple way to fix the break?

Ideally you can find a new command to accomodate the change, because...

While you say

 the role name was just ${rolename}, now it is ${user}.${rolename}.

I would counter that the name of a role is in fact the name of its directory. I could be wrong but the ${user}.${rolename} convention that we're both familiar with is only a loose convention at best. The way I tend to see roles used, especially third-party roles via ansible-galaxy, is to install e.g. yoshzz.postfix and then create company.postfix using yoshzz.postfix as a dependency.

The previous behavior of ansigenome, which this PR "fixed" (for some definition of "fixed" :)) normalized the names of both to simply "postfix", which of course breaks the dependency graph. I think it makes more sense to let the roles speak for themselves when it comes to naming.

Two points with all that in mind:
1. --limit is definitely broken for the export command and I've got a PR coming for that. It amounts to fixing a DRY-related omission, so
2. I wouldn't be surprised if the gendoc command suffers from a similar omission.

I hope we can find a way to make this work for you without trying to walk this back.

@ypid
Copy link
Contributor

ypid commented Jun 20, 2016

Thanks for your work. It is just that Ansigenome is used for the DebOps (example: https://github.com/debops/ansible-apt_install) and I think using only the $role_name from $role_owner.$role_name is preferred for the DebOps READMEs. @drybjed What do you think?

I could be wrong but the ${user}.${rolename} convention that we're both familiar with is only a loose convention at best.

According to the Ansible docs I see your point but with the existence of Ansible Galaxy, this ${user}.${rolename} has become the standard as far as I can tell.

The way I tend to see roles used, especially third-party roles via ansible-galaxy, is to install e.g. yoshzz.postfix and then create company.postfix using yoshzz.postfix as a dependency.

I don’t usually do this. The roles I use can usually be used as standalone role and if a modification is involved, I contribute the changes back. Does the company.postfix bring you an advantage?

@jimbocoder If you could make the name change optional/configurable, that would be awesome.

@jimbocoder
Copy link
Contributor Author

@ypid Could you open an issue to discuss this further please? If you could include an example or explanation of the broken rendering it would be helpful too. I've given a little thought to how to proceed but let's be more public so others can chime in. Tag me! Thanks.

@ypid
Copy link
Contributor

ypid commented Jun 21, 2016

Sure, no problem. #48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants