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

Put load_module directives before the stream block #84

Open
wants to merge 2 commits into
base: master
from

Conversation

@choroba
Copy link

@choroba choroba commented Nov 4, 2018

Fixes #69.

@choroba
Copy link
Author

@choroba choroba commented Nov 4, 2018

This is just a wild guess. Feel free to correct me if it's wrong or too hacky.

@@ -906,7 +906,7 @@ sub write_config_file ($$) {
if ($LoadModules) {
my @modules = map { "load_module $_;" } grep { $_ } split /\s+/, $LoadModules;
if (@modules) {
$main_config .= join " ", @modules;
substr $main_config, 0, 0, join " ", @modules;

This comment has been minimized.

@agentzh

agentzh Nov 4, 2018
Member

Maybe it's more readable to say $main_config = (join " ", @modules) . " " . $main_config?

@choroba choroba force-pushed the choroba:issue69 branch from 8fed0e9 to 2064823 Nov 5, 2018
@@ -906,7 +906,7 @@ sub write_config_file ($$) {
if ($LoadModules) {
my @modules = map { "load_module $_;" } grep { $_ } split /\s+/, $LoadModules;
if (@modules) {
$main_config .= join " ", @modules;
$main_config = join " ", @modules, $main_config;

This comment has been minimized.

@agentzh

agentzh Nov 5, 2018
Member

Better add a space between these two things to make the resulting nginx.conf look a bit better?

This comment has been minimized.

@agentzh

agentzh Nov 5, 2018
Member

Oh, never mind, a space is already used as a delimiter in the join() call :)

This comment has been minimized.

@thibaultcha

thibaultcha Nov 6, 2018
Member

This produces the following output:

load_module foo; load_module bar; stream {

    server {
        listen 1985;
        ...
    }
}

I would be in favor of having the following output, which makes debugging easier when inspecting the generated nginx.conf:

load_module foo;
load_module bar;
stream {

    server {
        listen 1985;
        ...
    }
}

This simple change is my suggestion:

Suggested change
$main_config = join " ", @modules, $main_config;
$main_config = join "\n", @modules, $main_config;
@agentzh
agentzh approved these changes Nov 5, 2018
@agentzh
Copy link
Member

@agentzh agentzh commented Nov 5, 2018

@thibaultcha Will you have a look at this. If it looks good to you, then feel free to merge this :) Thanks!

@agentzh
Copy link
Member

@agentzh agentzh commented Nov 6, 2018

One concern is the 1 offset in the line numbers for the subsequent lines. This may break existing tests.

@thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Nov 6, 2018

@agentzh Tests usually refer to the Lua snippet within the *_by_block directive though, don't they? Are there other tests that rely on the nginx.conf line numbers themselves? This might be unlikely, otherwise the insertion of the env directives would constantly offset the generated config from different systems running them, wouldn't they?

@agentzh
Copy link
Member

@agentzh agentzh commented Nov 6, 2018

@thibaultcha Some tests may reference nginx.conf line numbers printed in the error log messages, like this:

runtime error: access_by_lua(nginx.conf:37):2: bad bad bad
@thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Nov 6, 2018

@agentzh Indeed, I have found the following cases of tests such as you described:

./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]==]" in .*?\nginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:22/
./stream-lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]=]" in .*?\bnginx\.conf:22/
./lua-nginx-module/t/158-global-var.t:rewrite_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:access_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:content_by_lua\(nginx\.conf:48\):3: in main chunk, \n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:log_by_lua\(nginx\.conf:50\):3: in main chunk\n\z/, "old foo: 1\n"]
./lua-nginx-module/t/158-global-var.t:content_by_lua\(nginx\.conf:56\):4: in\n\z/, "old foo: 1\n"]
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]==]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/132-lua-blocks.t:qr/\[emerg\] .*? Lua code block missing the closing long bracket "]=]" in .*?\bnginx\.conf:41/
./lua-nginx-module/t/142-ssl-session-store.t:qr/\[emerg\] .*? "ssl_session_store_by_lua_block" directive is not allowed here .*?\bnginx\.conf:28/

If you don't mind, I would like to update them to use nginx\.conf:\d+ instead, since imho, relying on the nginx.conf line number is unsafe and should be subject to changes (what if we'd like to introduce a new directive, such as a new env variable in the future?). Third-party test suites (external to the OpenResty organization) should probably not assume that the internal nginx.conf is not subject to changes either...

Once updated, we can safely apply my suggestion and merge this. What do you think?

@agentzh
Copy link
Member

@agentzh agentzh commented Feb 21, 2019

@thibaultcha Actually some tests really want to check the nginx.conf line numbers. That's part of the things we want to check accurately.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.