Feat: yamlls

[qvalentin]

Testers needed

This is finally in the state where you can do some final testing before this can be merged:

Check out the updated documentation: https://github.com/qvalentin/helm-ls/tree/feat-yamlls

To test this branch you need to compile helm-ls yourself or download it from here; https://github.com/qvalentin/helm-ls/releases/tag/master:

git clone https://github.com/qvalentin/helm-ls.git
cd helm-ls
git checkout feat-yamlls
go build 
cp helm-ls /usr/local/bin/helm_ls # or where you have it installed

Feel free to ask if something is unclear, so I can improve the docs and report any bugs you find.

Original PR description

Integration of yamll.

This enables:

• schema validation
• completion for yaml
• hover for yaml

Also contains some other commits, so it must be rebased.

Some todos:

• add workarounds for false positives like: "All mapping items must start at the same column" caused by includes
• add workaround for multiple yaml documents in one file (I can't reproduce this bug, can you send me an example @MikaelElkiaer ?)
• add documentation for setting it up with yamlls, also explain stuff like schemas
• add config options (helmls does not have any lsp config options currently): Allow turning this feature off, configure which yaml schema should be used, location of the yamlls executable, etc.
• maybe add an error threshold, to suppress errors, if there are too many in one file
• Make completion work again

List of false positives of diagnostics:

• "All mapping items must start at the same column" with include or toYaml (solved by ignoring all diagnostics of this type)
• "Map keys must be unique" with if-statements
• "Property app.kubernetes.io/component is not allowed." (seems to be a bug in yamlls itself or the schema)
• "A block sequence may not be used as an implicit map key" (solved by ignoring all diagnostics of this type)
• "Implicit keys need to be on a single line" with toYaml (solved by ignoring all diagnostics of this type)
• "Implicit map keys need to be followed by map values" with toYaml (solved by ignoring all diagnostics of this type)

List of general bugs:

• Variable definitions like {{- $kube := "" -}}` are not trimmed correctly

Feature to add after this was merged:

• https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_documentSymbol
• https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_foldingRange
• indent and auto formatting?

[mrjosh]

@qvalentin Can you please resolve the conflicts and golang ci lint errors so we can merge the pr?

[qvalentin]

This still has some major bugs I need to resolve before it is ready.

[MikaelElkiaer]

This seems like a very cool change, but a bit hard for me to try out due to the conflicts.

Do you have a ref where you have this PR, and possibly also #24, applied on top of mrjosh/helm-ls/tree/master ?

[qvalentin]

This seems like a very cool change, but a bit hard for me to try out due to the conflicts.

Do you have a ref where you have this PR, and possibly also #24, applied on top of mrjosh/helm-ls/tree/master ?

I will probably need one or two more weekends till it will be in a useable state.

[MikaelElkiaer]

I will probably need one or two more weekends till it will be in a useable state.

No problem.
Would love to help, at least by trying it out and perhaps weeding out some bugs.

[qvalentin]

Here is some information about the current state and the problems of this, maybe you have some ideas:

The currently implemented approach takes the yaml from the helm template and tries to remove everything that is not valid yaml and produces no output when the template is rendered. For example: It removes the control structures of an if, but keeps the content of an if. For more examples check out the tests.
This works great for the control structures.

Everything that would be replaced when the template is being rendered is kept in the trimmed yaml. When diagnostics are sent from yamlls those are filtered and all diagnostics that relate to a template (everything inside {{}}) are removed.

This was relatively easy to implement and works for most templates. The important part is that the positions (line and char) of the yaml are not changed, when it is trimmed. This way yamlls and helmls use the same positions for all lsp operations (e.g. hover, completion, diagnostics).

Problematic is everything where complex yaml structures are produced when rendering the template. So include and toYaml produce a trimmed yaml that is not correct.
This is mostly problematic for diagnostics as false positives are produced.

I have two ideas of how this could be avoided:

1. Filter out all diagnostic that could be false positives. Most of them are related to indentation.
   This way we would also loose some useful diagnostics. Keeping track of which diagnostics are problematic could also be difficult.
2. Use a rendered yaml instead of a trimmed yaml. This way we would have to translate the positions from the template to the rendered yaml for every lsp operation. This translation would also be very difficult to implement.

Overall this was very interesting to implement and maybe we have to make a few compromises. All features using yamlls will be "best-effort" and won't guarantee correctness.

[MikaelElkiaer]

Got it up and running now, will try it out and see if I can contribute.

For anyone else tuning in, apply this patch after checking out:

diff --git a/internal/adapter/yamlls/yamlls.go b/internal/adapter/yamlls/yamlls.go
index 304abb5..b1e2d78 100644
--- a/internal/adapter/yamlls/yamlls.go
+++ b/internal/adapter/yamlls/yamlls.go
@@ -17,8 +17,8 @@ type YamllsConnector struct {
 }
 
 func NewYamllsConnector(workingDir string, clientConn jsonrpc2.Conn, documents *lsplocal.DocumentStore) *YamllsConnector {
-	// yamllsCmd := exec.Command("yaml-language-server", "--stdio")
-	yamllsCmd := exec.Command("debug-lsp.sh")
+	yamllsCmd := exec.Command("yaml-language-server", "--stdio")
+	// yamllsCmd := exec.Command("debug-lsp.sh")
 
 	stdin, err := yamllsCmd.StdinPipe()
 	if err != nil {

I also had to update my PATH in order to make mason.nvim binaries available.
I bet something could be done to grab the configured command from lspconfig and use that though, something like:

$ nvim --headless -c 'lua =table.concat(require("lspconfig.configs").yamlls.cmd, " ")' +qa
/home/me/.local/share/nvim/mason/bin/yaml-language-server --stdio

[MikaelElkiaer]

Hey, I've been using this for some weeks now.
I want to provide some more thorough feedback, but just went on vacation.
Let me jot down a little before I forget.

1. It generally works as one would expect - awesome!
2. It does not work for files containing multiple yaml files - i.e. blocks separated by ---.
3. It would be nice with further yaml-specific LSP features, such as formatting.

But all in all, awesome work!
Will come back to you once I am back from vacation and give some more details.

[smaftoul]

Any news on this ? Can we hope this get merged, even if not completely working, we currently have no proper editing experience for helm chart, it's either gotemplate which doesn't do a lot, or yamlls, which is plenty of errors because of gotemplate.

[mrjosh]

Any news on this ? Can we hope this get merged, even if not completely working, we currently have no way have proper editing experience for helm chart, it's either gotemplate which doesn't do a lot, or yamlls, which is plenty of errors because of gotemplate.

Hi there hope you're doing well.
I'm super busy to checkout these changes + we need to resolve the conflicts on this pr.
As soon as the conflicts are resolved, i can merge the pr.

[qvalentin]

Any news on this ? Can we hope this get merged, even if not completely working, we currently have no way have proper editing experience for helm chart, it's either gotemplate which doesn't do a lot, or yamlls, which is plenty of errors because of gotemplate.

Hi there hope you're doing well. I'm super busy to checkout these changes + we need to resolve the conflicts on this pr. As soon as the conflicts are resolved, i can merge the pr.

Hi, nice to hear from you again @mrjosh.

I'm a bit reluctant to put more work into this MR (and there's still a lot of work to be done) when I don't know when I'll get the chance to merge it. If you would authorize me to push on the project, that would increase my motivation. Otherwise I would probably set up my own fork. This also refers to the other MRs, most of which are ready to merge for some time already. Those should be looked at before moving on here.

[mrjosh]

Any news on this ? Can we hope this get merged, even if not completely working, we currently have no way have proper editing experience for helm chart, it's either gotemplate which doesn't do a lot, or yamlls, which is plenty of errors because of gotemplate.

Hi there hope you're doing well. I'm super busy to checkout these changes + we need to resolve the conflicts on this pr. As soon as the conflicts are resolved, i can merge the pr.

Hi, nice to hear from you again @mrjosh.

I'm a bit reluctant to put more work into this MR (and there's still a lot of work to be done) when I don't know when I'll get the chance to merge it. If you would authorize me to push on the project, that would increase my motivation. Otherwise I would probably set up my own fork. This also refers to the other MRs, most of which are ready to merge for some time already. Those should be looked at before moving on here.

Hi, thanks for the work you put into this project
Unfortunately, i can't put work into this project at the moment, maybe next month
But yeah, of cource. I can't be more happier to give you access to the project
I've invited you already

[smaftoul]

@mrjosh thanks for answering and allowing allowing @qvalentin to push to the repo !

@qvalentin if you need some help / tester ... don't hesitate ;)

[qvalentin]

If anyone has experience with tree-sitter grammars: Help with this: ngalaiko/tree-sitter-go-template#5 would be appreciated.

[MikaelElkiaer]

If anyone has experience with tree-sitter grammars: Help with this: ngalaiko/tree-sitter-go-template#5 would be appreciated.

A little experience, but I would love to get this working, so I will try and have a look once I get around to it.

[ejhayes]

This looks super helpful @qvalentin - do you know if this would add support for subcharts (or at least make implementing that functionality a little easier?)

[qvalentin]

This looks super helpful @qvalentin - do you know if this would add support for subcharts (or at least make implementing that functionality a little easier?)

Hi @ejhayes, this is not related to subcharts, adding that feature would still take the same steps. I added some more info to #46

[qvalentin]

This is ready for a final round of testing, I updated the description of this PR.

[MikaelElkiaer]

Awesome job, well done!

I see have a un-checked box with my name on it, so I will try out the latest version ASAP. :)

[MikaelElkiaer]

Looking very good!
I don't see any issues with multiple documents in one file, so you can tick that off.

One question though, how do you plan on making the tree-sitter grammar available?
I see you have a forked repo, and have the parser included in this PR.

[qvalentin]

Looking very good! I don't see any issues with multiple documents in one file, so you can tick that off.

One question though, how do you plan on making the tree-sitter grammar available? I see you have a forked repo, and have the parser included in this PR.

Nice, thanks for testing.

Regarding tree-sitter, I'm planning to upstream all changes to the grammar to https://github.com/ngalaiko/tree-sitter-go-template.

I haven't really looked into using the grammar in neovim directly, but this is definitely something we should include in the Readme, would be related to #33.

[cenk1cenk2]

I have also tested this since you changed the title to testers wanted. I would say it is working pretty great, really do appreciate your time on this, it is a very good feature to have.

[qvalentin]

Thanks to everyone who tested :)

[cenk1cenk2]

Can not thank you enough for your work on this!

[mrjosh]

@cenk1cenk2 ikr? @qvalentin is a legend. :)
Thanks guys for putting your time into this
Unfortunately i can't find some time to work on this project
But you guys keeping this project alive
I appreciate it