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

[refactor] drop pcre regexes for lua patterns #209

Merged
merged 5 commits into from
May 9, 2015
Merged

Conversation

thibaultcha
Copy link
Member

Following the discussion we had in #167, this PR removes the need for patterns everywhere accross Kong.

  • This PR does not yet remove the installation of perl nor pcre during the package building or the travis-ci setup. We will do it if we decide to merge this. For the time being, this PR only shows what would change by using patterns.
  • There should be no major performance drawback by using patterns instead of Perl regexps as we are not doing heavy text processing at all (at least the core and current plugins). And lua patterns can also be nicer than Perl regexps for some use cases.
  • As a side effect, regexes are assumed to disappear entirely once those are removed, and the regex field in schemas is not supported anymore. One now needs to use func and use Lua patterns.

Finally, I would say I really like the idea of lightening the crazy amount of dependencies Kong requires, but I am not yet 100% sure about that one. pcre is recommended by openresty here (sure, we can probably drop it if we don't use it), but future plugins might eventually want to use Perl regexps, which this will be preventing. However, plugins (and the core) could still use powerful lua pattern libraries (LPeg) if they feel the need.

I think it's a discussion we should have before merging this @montanaflynn @thefosk.

@thibaultcha thibaultcha force-pushed the refactor/patterns branch from 9fce0df to 93fd629 Compare May 8, 2015 00:55
@montanaflynn
Copy link

Removing PCRE as a dependency would prevent anyone from using regular expressions in their plugins? Would we want to use it for handling regex based routing? Even if we're not using it now or in the future I think it should be available for third-party plugin authors.

@thibaultcha thibaultcha force-pushed the refactor/patterns branch from 93fd629 to a933c54 Compare May 8, 2015 01:22
@thibaultcha
Copy link
Member Author

Patterns can also do a great job. It depends what we want to use. This (or at least a part of it) can still be merged (it's lighter than what we used to have, especially for the specs) and it doesn't remove the actual compilation with pcre (as said in the PR).

If we don't remove pcre, then #167 is irrelevant.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.32% when pulling a933c54 on refactor/patterns into 8acfe1d on master.

- switch `public_dns` validation from regex to a custom function with
  pattern validation (in its schema)
- remove regex field in schemas and schemas.lua
- uuid validation now happens with patterns in base_dao.lua
This does not remove the installation of pcre in the package build or the
openresty installation of travis-ci.
@thibaultcha thibaultcha force-pushed the refactor/patterns branch 3 times, most recently from cf94bce to 46525b5 Compare May 9, 2015 14:57
@thibaultcha thibaultcha force-pushed the refactor/patterns branch from 46525b5 to 2cf80ea Compare May 9, 2015 15:11
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 663e130 on refactor/patterns into * on master*.

thibaultcha added a commit that referenced this pull request May 9, 2015
[refactor] drop pcre regexes for lua patterns
@thibaultcha thibaultcha merged commit cf86063 into master May 9, 2015
@thibaultcha thibaultcha deleted the refactor/patterns branch May 9, 2015 16:35
ctranxuan pushed a commit to streamdataio/kong that referenced this pull request Aug 25, 2015
[refactor] drop pcre regexes for lua patterns
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.

3 participants