pappasam / nginx-language-server

Language server for nginx.conf
GNU General Public License v3.0
60 stars 2 forks source link

Support fragmented Nginx configuration #11

Open harry-xm opened 3 years ago

harry-xm commented 3 years ago

Common practice, as seen in default configuration on Ubuntu, is to split server{} blocks into separate files in the sites-enabled directory.

When I try to use nginx-language-server on these partial configuration files, crossplane could not correctly parse them because of errors like "server" directive is not allowed here.

Is it possible to detect such configuration and automatically add a surrounding block to fix crossplane parsing?

# /etc/nginx/sites-enabled/default
server {
    listen 80 default_server;
    listen [::]:80 default_server;
...
}

# nginx.conf
http {
...
    include /etc/nginx/conf.d/*.conf;
    include /etc/nginx/sites-enabled/*;
}
pappasam commented 3 years ago

Hmm, I'm not quite sure here. Will take some considerable digging. If you'd like to take a stab in the meantime, it looks like crossplane.parse takes a combine argument that's False by default. Setting it to True may get you on the right track.

pappasam commented 3 years ago

Actually, are you asking for completion within /etc/nginx/sites-enabled/default? In that case, I have no idea where to start there... if you have any ideas please let me know!

pappasam commented 3 years ago

Maybe we can do something with comments? We'd need some way to generally identify where an included script is intended to run. I think this would need to be communicated from the user to the language server somehow.

Alternatively, we could somehow identify whether there is a root nginx.conf and use that to determine the location of included files and figure out their context. Not sure what would be best here, will definitely take some design work.

harry-xm commented 3 years ago

Actually, are you asking for completion within /etc/nginx/sites-enabled/default? In that case, I have no idea where to start there... if you have any ideas please let me know!

Yes. I did a quick hack, but it would break other configurations. I'm thinking of checking for directive is not allowed here error messages and redo parsing with such wrapping to see if those errors are gone.

diff --git a/nginx_language_server/parser/nginxconf.py b/nginx_language_server/parser/nginxconf.py
index 2480be3..fb11b36 100644
--- a/nginx_language_server/parser/nginxconf.py
+++ b/nginx_language_server/parser/nginxconf.py
@@ -42,8 +42,8 @@ def convert(code: str) -> ParsedSymbols:
     """Load and parse symbols into a container for future processing."""
     try:
         with NamedTemporaryFile(mode="w", delete=False) as nginx:
-            nginx.write(code)
-        parsed = crossplane.parse(nginx.name)["config"][0]["parsed"]
+            nginx.write("http{"+code+"}")
+        parsed = crossplane.parse(nginx.name, single=True)["config"][0]["parsed"]
         result: ParsedSymbols = {}
         _convert(result, parsed, [])
         return result
harry-xm commented 3 years ago

If crossplane supports parsing config within a context, the implementation would be much more elegant. I think it's worth asking.

harry-xm commented 3 years ago

We could use check_ctx=False to make crossplane.parse() work, and replace the default root context "main" with smart inference of the most probable context.

pappasam commented 3 years ago

Sounds like an interesting approach! Can't say I fully see a clear path forward, but I'll keep meditating and will happily review any PR's if you discover a viable path.