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

Allow $ in treetop parsed strings. #241

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

kwolf-zz
Copy link
Contributor

I'm in the process of upgrading from Puppet 3 to 4 and using this as an opportunity to upgrade to the latest wildfly module as well.

I had some parsing problems with Treetop, and the only ones I couldn't fix on our side were strings that had $ in them. I tried quoting them, escaping them everything I could think of but couldn't get it to pass Treetop's validation.

This PR allows dollar signs. I'd love to get this accepted... this is the only real reason we'll have to run our fork.

The strings I had issue with are:
/subsystem=logging/logger=com.company.common.services.DefaultCompanyPoolExecutor$UserBasedCallable:dummy /subsystem=logging/logger=com.company.common.services.DefaultCompanyThreadPoolExecutor$UserBasedRunnable:dummy

I tried this all the way up to master of the Puppet module. If I'm missing something and need to escape this differently or in a way I haven't though of, please let me know.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 74.385% when pulling a8d9711 on kwolf:dollar_sign into acba51c on biemond:master.

@coveralls
Copy link

coveralls commented May 25, 2018

Coverage Status

Coverage remained the same at 74.385% when pulling c75c40d on kwolf:dollar_sign into acba51c on biemond:master.

@kwolf-zz
Copy link
Contributor Author

kwolf-zz commented Jun 1, 2018

Treetop was also failing one a line similar too:

command => '/subsystem=mail/mail-session=default:write-attribute(name=from, [email protected])',

Adding @ and $ fix all the strings we're having issues with after moving to Puppet 4 and the latest version of your module.

Please let me know if this can be pulled into master.

@cqwense
Copy link
Contributor

cqwense commented Jun 6, 2018

Is there any chance this is related to #215 which is using the # character ?

edit: adding # to the allowed characters does indeed resolve this issue. I've opened kwolf-zz#1 in the hopes it can get included - if not no worries i'll open a new one here.

@kwolf-zz
Copy link
Contributor Author

kwolf-zz commented Jun 7, 2018

Merged the # into my PR.

@kwolf-zz
Copy link
Contributor Author

kwolf-zz commented Jun 7, 2018

I don't think that last failure was related to the code change.

@kwolf-zz
Copy link
Contributor Author

@biemond any update on possibly merging my three PR's? This is the most important one.

@jairojunior jairojunior reopened this Aug 15, 2018
@jairojunior
Copy link
Collaborator

Thanks.

@jairojunior jairojunior merged commit b0a8ec8 into voxpupuli:master Aug 15, 2018
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.

5 participants