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

Add lua analyzer to stream and upstream #103

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rikatz
Copy link

@rikatz rikatz commented Jul 8, 2024

Proposed changes

Some directives are missing on NGINX stream, and some other inside main HTTP server.

This PR fixes the missing directives

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md)

@rikatz rikatz requested a review from a team as a code owner July 8, 2024 21:54
@ornj
Copy link
Member

ornj commented Jul 12, 2024

Thank you for submitting a PR @rikatz.

The current set of Lua directives supported by crossplane focus on supporting ngx_http_lua_module. I think the changes you're making are releated to ngx_stream_lua_module.

@xynicole @dengsh12 Should we support this module by merging the flags into moduleLuaDirectives or do we want to model it as a separate optional module?

@xynicole
Copy link
Collaborator

@ornj I am leaning towards the second option as it provides better modularity and clarity. Additionally, since it involves two separate repositories, it would be easier to use a generate approach rather than manually adding the directives. Furthermore, since the ngx_stream_lua_module is not distributed with NGINX by default, using it as a separate optional module allows us to avoid affecting the existing ngx_http_lua_module directives.

@dengsh12
Copy link
Collaborator

dengsh12 commented Jul 12, 2024

Thank you for submitting a PR @rikatz.

The current set of Lua directives supported by crossplane focus on supporting ngx_http_lua_module. I think the changes you're making are releated to ngx_stream_lua_module.

@xynicole @dengsh12 Should we support this module by merging the flags into moduleLuaDirectives or do we want to model it as a separate optional module?

I thought in a separate file is easier for management and make more sense. It seems they are registered as two modules in fact(different ngx_module_t). If we merge into one file it comes to a new special case(and seems a bit weird) for the code generator. The relationships among ParseOptions.DirectiveSources is or so put them in two files won't hurt any valid config files.

After the go generate tool available it should be easy to add & keep update for it. Since we have specialize bitmasks for block directives in http_lua_module, I wonder if we also need some special operations for this module?

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.

4 participants