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 Helm extension #746

Merged
merged 10 commits into from
Oct 15, 2024
Merged

Add Helm extension #746

merged 10 commits into from
Oct 15, 2024

Conversation

cabrinha
Copy link
Contributor

@cabrinha cabrinha commented May 17, 2024

Screenshot 2024-05-17 at 3 50 00 PM

I've got this extension to a point now where all highlights seem to be working properly, but there are still a couple issues I need some help with:

  • Files aren't automatically detected as "Helm", but instead as "YAML", need to find a way to detect helm if the *.yaml file is located in a directory named templates: templates/*.yaml, is this possible?
  • Some errors are being displayed as YAML errors, but really its valid Helm syntax

Screenshot 2024-05-17 at 3 53 37 PM

@cla-bot cla-bot bot added the cla-signed label May 17, 2024
@cabrinha cabrinha changed the title Helm: use tree-sitter-go-template dialect Adding Helm: use tree-sitter-go-template "helm" dialect May 17, 2024
@cabrinha cabrinha mentioned this pull request May 17, 2024
1 task
@maxdeviant maxdeviant changed the title Adding Helm: use tree-sitter-go-template "helm" dialect Add Helm extension May 23, 2024
@aohoyd
Copy link

aohoyd commented Jun 3, 2024

Files aren't automatically detected as "Helm", but instead as "YAML", need to find a way to detect helm if the .yaml file is located in a directory named templates: templates/.yaml, is this possible?

I don't think templates/*.yaml is enough. Personally I use templates directory for my projects as a base directory for templates not related to helm. Imo it should check for a file Chart.yaml in the parent directory of templates/ and only then recognise templates/**/*.(yaml|yml|tpl) files as go templates. I'm not sure how to do it in Zed, trying to figure out

Some errors are being displayed as YAML errors, but really its valid Helm syntax

These errors come from original YAML language server. You can disable it and errors will be gone.

@yodatak
Copy link

yodatak commented Jun 20, 2024

Any news on this would love to use it ^^ for more kubernetes with rust

@cabrinha
Copy link
Contributor Author

Any news on this would love to use it ^^ for more kubernetes with rust

Need some help on getting this to "work". First up, the CI tests are failing, but this extension mostly works. Second...

@yodatak do you know how to get .yaml files recognized as Helm? Either the existence of a Chart.yaml file or the files exist inside of a templates directory? Maybe the existence of {{ }} in a .yaml is enough to switch the syntax from YAML to Helm?

@selfisch
Copy link

Any news on this would love to use it ^^ for more kubernetes with rust

Need some help on getting this to "work". First up, the CI tests are failing, but this extension mostly works. Second...

@yodatak do you know how to get .yaml files recognized as Helm? Either the existence of a Chart.yaml file or the files exist inside of a templates directory? Maybe the existence of {{ }} in a .yaml is enough to switch the syntax from YAML to Helm?

Hey there, thanks for working on this, great work so far. Unfortunately I cannot support you.

But just said, {{ }} is not automatically Helm. I am using a tool which has Jinja2 templating, so I can template my kubernetes manifests with it. I have also this issue of bracket space with Zed not only, but also for Helm.

@runitmisra
Copy link

@cabrinha What I have generally seen from neovim autocmds to load helm-ls as language server automatically, is that it looks for the following:

  • The yaml or tpl file should be in a templates folder
  • There should be a Chart.yaml in the project root directory (same level as templates folder)

If both are true, it is pretty safe to assume user is working with Helm. Hope this helps.

@cabrinha
Copy link
Contributor Author

@cabrinha What I have generally seen from neovim autocmds to load helm-ls as language server automatically, is that it looks for the following:

  • The yaml or tpl file should be in a templates folder
  • There should be a Chart.yaml in the project root directory (same level as templates folder)

If both are true, it is pretty safe to assume user is working with Helm. Hope this helps.

Thanks for those ideas, but how does that implementation look in Zed?

@gnikishin
Copy link

gnikishin commented Jul 15, 2024

a bit of nuance, sometimes Chart.yaml isn't located in project root.
e.g /project_root/dir/.helm/Chart.yaml and /project_root/dir/.helm/templates/

UPD. also there is related issue, which will make working with helm kinda painful
#12122

@mikeymop
Copy link

mikeymop commented Jul 18, 2024

@yodatak
Have you gotten that to work?
I actually asked for help about that in 14528

I believe this is correct according to both documentations

{
  "lsp": {
    "yaml-language-server": {
      "initialization_options": {
        "yaml.schemas": {
          "http://json.schemastore.org/chart.json": "templates/*.yaml"
        }
      }
    }
  }
}
{
  "lsp": {
    "yaml-language-server": {
      "initialization_options": {
        "yaml": {
          "schemas": {
            "http://json.schemastore.org/chart.json": "templates/*.yaml"
          }
        }
      }
    }
  }
}

However I could only apply a schema with inline comments

# yaml-language-server: $schema=https://json.schemastore.org/chart.json

@yodatak
Copy link

yodatak commented Jul 31, 2024

I think we need to fix zed-industries/zed#7636 before merging ? i would love to have more kubernetes autocompletion with flux2 and so one but

@yodatak
Copy link

yodatak commented Jul 31, 2024

@yodatak Have you gotten that to work? I actually asked for help about that in 14528

I believe this is correct according to both documentations

{
  "lsp": {
    "yaml-language-server": {
      "initialization_options": {
        "yaml.schemas": {
          "http://json.schemastore.org/chart.json": "templates/*.yaml"
        }
      }
    }
  }
}
{
  "lsp": {
    "yaml-language-server": {
      "initialization_options": {
        "yaml": {
          "schemas": {
            "http://json.schemastore.org/chart.json": "templates/*.yaml"
          }
        }
      }
    }
  }
}

However I could only apply a schema with inline comments

# yaml-language-server: $schema=https://json.schemastore.org/chart.json

I don't find any way seem to be a bug from yaml lsp i think

for me for flux2 i need to do

# yaml-language-server: $schema=https://raw.githubusercontent.com/fluxcd-community/flux2-schemas/main/helmrelease-helm-v2.json
---
apiVersion: helm.toolkit.fluxcd.io/v2

or for kustomisation

# yaml-language-server: $schema=https://json.schemastore.org/kustomization.json
apiVersion: kustomize.config.k8s.io/v1beta1

or for namespace

# yaml-language-server: $schema=https://kubernetesjsonschema.dev/v1.14.0/Namespace-v1.json
---
apiVersion: v1
kind: Namespace

@aohoyd
Copy link

aohoyd commented Aug 6, 2024

I think that LanguageMatcher should be extended with something like path_globs to match not only file suffixes but also subpaths

@notpeter
Copy link
Member

notpeter commented Aug 6, 2024

I think that LanguageMatcher should be extended with something like path_globs to match not only file suffixes but also subpaths

I think you should be able to use ** which will match 0 or more segments across directories. So maybe something like: templates/**/*.yaml?

@cabrinha cabrinha force-pushed the helm-ext branch 2 times, most recently from 5e2e32f to b685047 Compare August 9, 2024 05:44
@cabrinha
Copy link
Contributor Author

cabrinha commented Aug 9, 2024

I think that LanguageMatcher should be extended with something like path_globs to match not only file suffixes but also subpaths

I think you should be able to use ** which will match 0 or more segments across directories. So maybe something like: templates/**/*.yaml?

Tried to do this here: cabrinha/helm.zed@6f2ac46 ... Still not working.

@nabiltntn
Copy link

Files aren't automatically detected as "Helm", but instead as "YAML", need to find a way to detect helm if the .yaml file is located in a directory named templates: templates/.yaml, is this possible?

I don't think templates/*.yaml is enough. Personally I use templates directory for my projects as a base directory for templates not related to helm. Imo it should check for a file Chart.yaml in the parent directory of templates/ and only then recognise templates/**/*.(yaml|yml|tpl) files as go templates. I'm not sure how to do it in Zed, trying to figure out

Some errors are being displayed as YAML errors, but really its valid Helm syntax

These errors come from original YAML language server. You can disable it and errors will be gone.

I think also that some charts also create sub-charts under the charts folder, so I think this should also be include in the pattern

@cabrinha
Copy link
Contributor Author

I think also that some charts also create sub-charts under the charts folder, so I think this should also be include in the pattern

Those charts are tar'd up, so they don't really need to be opened and rendered.

@selfisch
Copy link

Hey,
discussing what files need to be edited and which not is not purposeful in this case.

Like mentioned many posts above, this is no explicit helm issue, but more like a templating include issue in the yaml lsp.
I am using jinja2 templating in yaml files and have almost the same issue, no matter which ide using.
There needs to be a way to include templating definitions like go/sprig for helm, but also jinja2 for example, into the yaml lsp definition. And that makes the whole topic more komplicated in general :-/

@gnikishin
Copy link

Those charts are tar'd up, so they don't really need to be opened and rendered.

not always, it can be custom sub chart which written in plain yaml files.

This directory "multipurpose", subcharts can be in form of tar.gz, and in form of directory

@cabrinha
Copy link
Contributor Author

Those charts are tar'd up, so they don't really need to be opened and rendered.

not always, it can be custom sub chart which written in plain yaml files.

This directory "multipurpose", subcharts can be in form of tar.gz, and in form of directory

This is all valid, but how do we get closer to getting Zed to support helm syntax?

@mikeymop
Copy link

mikeymop commented Aug 22, 2024

Those charts are tar'd up, so they don't really need to be opened and rendered.

not always, it can be custom sub chart which written in plain yaml files.

This directory "multipurpose", subcharts can be in form of tar.gz, and in form of directory

This is all valid, but how do we get closer to getting Zed to support helm syntax?

I would argue that properly supporting jsonschema overrides in yaml-language-server is the best place to divert focus towards.

It is working with an inline comment today, if it can be made to work in settings.json, and with glob patterns, it would be comparable what VsCode offers.

@cabrinha
Copy link
Contributor Author

Those charts are tar'd up, so they don't really need to be opened and rendered.

not always, it can be custom sub chart which written in plain yaml files.
This directory "multipurpose", subcharts can be in form of tar.gz, and in form of directory

This is all valid, but how do we get closer to getting Zed to support helm syntax?

I would argue that properly supporting jsonschema overrides in yaml-language-server is the best place to divert focus towards.

It is working with an inline comment today, if it can be made to work in settings.json, and with glob patterns, it would be comparable what VsCode offers.

Not sure I have much to offer there... Should we close this PR?

@aohoyd
Copy link

aohoyd commented Sep 17, 2024

It is working with an inline comment today, if it can be made to work in settings.json, and with glob patterns, it would be comparable what VsCode offers.

@mikeymop I didn't get what did you mean on what VsCode offers. VsCode offers separated extension which supports Helm syntax and it's features. The same is proposed here.

Moreover jsonschema provided above by @yodatak is a schema of Chart.yaml file, it doesn't automagically adds support of gotemplates syntax used by helm.

@cabrinha Here is what we need to do to get Helm support in Zed (from my point of view). We need some option, that would tell Zed to use Helm language for files stored in templates/** directories. I've seen this PR, but it doesn't support file path, so it should be somehow extended to also support full paths. I'll try to take a look there

@aohoyd
Copy link

aohoyd commented Oct 8, 2024

@cabrinha I figured it out. By some reason path_suffixes option in config.toml doesn't accept globs. You can specify

  "file_types": {
    "Helm": [
      "**/templates/**/*.tpl",
      "**/templates/**/*.yaml",
      "**/templates/**/*.yml"
    ]
  }

in your local .zed/settings.json config and get Helm language auto detected in projects. It works perfectly for me now, thank you!

@cabrinha
Copy link
Contributor Author

cabrinha commented Oct 9, 2024

@cabrinha I figured it out. By some reason path_suffixes option in config.toml doesn't accept globs. You can specify

  "file_types": {
    "Helm": [
      "**/templates/**/*.tpl",
      "**/templates/**/*.yaml",
      "**/templates/**/*.yml"
    ]
  }

in your local .zed/settings.json config and get Helm language auto detected in projects. It works perfectly for me now, thank you!

@aohoyd nice find, but why is the CI still failing?

What needs to happen in this PR to get this extension merged in and natively supported in Zed?

@notpeter
Copy link
Member

notpeter commented Oct 9, 2024

@cabrinha I figured it out. By some reason path_suffixes option in config.toml doesn't accept globs.
What needs to happen in this PR to get this extension merged in and natively supported in Zed?

Since those globs are not supported can you remove them from here:
languages/helm/config.toml and update the submodule commit?

We should also add a helm to the official languages docs including the suggested file_types configuration for Helm.

I fixed the sort order so tests should pass now.

@hedgieinsocks
Copy link

Thank you, it does work! But I encourage everyone who's waiting for this to be merged, in the meantime, to also visit the upstream issue ngalaiko/tree-sitter-go-template#23 and motivate the dev to review and fix it. Since it's likely that your helm charts use dash (-) to combine 2 templates into a single value.

@hedgieinsocks
Copy link

@cabrinha you might want to add "**/helmfile.d/**/*.yaml" to the common patterns when adding commit to the docs as suggested above

@cabrinha
Copy link
Contributor Author

@cabrinha I figured it out. By some reason path_suffixes option in config.toml doesn't accept globs.
What needs to happen in this PR to get this extension merged in and natively supported in Zed?

Since those globs are not supported can you remove them from here: languages/helm/config.toml and update the submodule commit?

We should also add a helm to the official languages docs including the suggested file_types configuration for Helm.

I fixed the sort order so tests should pass now.

I'm sure I will be flamed for "doing it wrong" but, here you go: zed-industries/zed#19095

@notpeter notpeter self-assigned this Oct 11, 2024
@qvalentin
Copy link

@cabrinha nice work :D.
I'm the current maintainer of helm-ls and also contributor to tree-sitter-go-template and would love to see them being used in zed.


Here are some of my viewpoints for the discussed topics:

Like mentioned many posts above, this is no explicit helm issue, but more like a templating include issue in the yaml lsp.

@selfisch yaml-language-server will probably never implement support for templating languages, it would be better to use a specific lsp for the template language like helm-ls.


I would argue that properly supporting jsonschema overrides in yaml-language-server is the best place to divert focus towards.

@mikeymop this PR may be interesting for you: redhat-developer/yaml-language-server#962


Thank you, it does work! But I encourage everyone who's waiting for this to be merged, in the meantime, to also visit the upstream issue ngalaiko/tree-sitter-go-template#23 and motivate the dev to review and fix it. Since it's likely that your helm charts use dash (-) to combine 2 templates into a single value.

@hedgieinsocks I just opened tree-sitter-grammars/tree-sitter-yaml#12 for this, as its a problem with the yaml tree-sitter grammar. If anyone has some tree-sitter and/or C knowledge you could help there.

Copy link
Member

@maxdeviant maxdeviant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work on this! Glad to see it finally land 😄

@maxdeviant maxdeviant merged commit 4ebc242 into zed-industries:main Oct 15, 2024
2 checks passed
maxdeviant added a commit to zed-industries/zed that referenced this pull request Oct 15, 2024
Merge after this:
- zed-industries/extensions#746

Release Notes:

- N/A

---------

Co-authored-by: Peter Tripp <[email protected]>
Co-authored-by: Marshall Bowers <[email protected]>
noaccOS pushed a commit to noaccOS/zed that referenced this pull request Oct 19, 2024
Merge after this:
- zed-industries/extensions#746

Release Notes:

- N/A

---------

Co-authored-by: Peter Tripp <[email protected]>
Co-authored-by: Marshall Bowers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.