towolf / vim-helm

vim syntax for helm templates (yaml + gotmpl + sprig + custom)
Other
200 stars 22 forks source link

Improve ftdetect #7

Closed rafi closed 2 months ago

rafi commented 5 years ago

Currently there could be collisions with regular yaml files and Ansible yamls, etc. Also, there's no support for yml, only yaml.

This can be improved by detecting Chart.yaml in parent directory for example. Here's an example from an Ansible plugin: https://github.com/pearofducks/ansible-vim/blob/master/ftdetect/ansible.vim

towolf commented 5 years ago

So, we also use Ansible and the distinction for us is easy, because our Ansible YAML files have .yml and Kubernetes YAMLs have .yaml.

But I guess that cannot be a universal distinction.

Detecting Chart.yaml would work but has some drawbacks, i.e. it presupposes a fully formed Helm dir structure.

Want to make a PR for this?

towolf commented 5 years ago

In the beginning I did not even include ftdetect at all, because I only wanted to share the syntax, and not the config. The latter is trickier and subjective. For instance, here's my personal ansibe detection logic:

autocmd BufRead,BufNewFile */ansible/*.yml  set ft=yaml.ansible
autocmd BufRead,BufNewFile *.j2,*/ansible/*/templates/* set ft=config.jinja2
nickjj commented 3 years ago

What if we also included a search on the file contents itself?

Something like this:

au BufNewFile,BufRead *.{yaml,yml} if getline(1) =~ '^apiVersion:' || getline(2) =~ '^apiVersion:' | setlocal filetype=helm | endif

I'm still fairly new to Kubernetes and Helm but isn't it a requirement that a Kubernetes manifest starts with either --- or apiVersion:? That doesn't help us with Chart.yml or values.yml (technically this can have Helm variables in it too right?) unless we specifically hard code those file names, but we would also want to account for values*.y*l because you might have a values-prod.yml file too.

But it's one step closer to figuring out what to do without depending on templates/? I've written about 40 Ansible roles and probably 10k+ lines of YAML at this point for it and I've never had a file named Chart.yml or values.yml so I don't think we'll hit any false positives or have conflicts with Ansible if we hard code those names. Maybe we'll run into false positives with values*.y*l for non-Ansible related yaml but this seems like an edge case that an end user could workaround, while everything else works great for the rest of us.

Perhaps this plugin could expose an optional setting where the user can define the regex matches for things like values*.y*l in case they decide to use a completely different naming convention too.

eshepelyuk commented 2 years ago

Hello @nickjj

  1. --- has nothing to do with k8s, it's just YAML separator for multi document in the single yaml file.
  2. But the idea of using apiVersion and kind - has a very high probability that yaml file contains K8S manifest.
nickjj commented 2 years ago

Yes, the --- is optional. That's why the snippet in my previous comment checks to see if apiVersion is the first or second line.

dannylongeuay commented 2 years ago

One other idea would be to do a quick check on file contents for any template variables. Search for a pattern, {{.+}}, and if found, set file type to helm. You want to make sure you only do this check on specific file types in the first place. I do this on yaml file types right now to work around LSP and tree-sitter issues.

luisdavim commented 2 months ago

What about something like:

function! s:isHelm()
  let filepath = expand("%:p")
  let filename = expand("%:t")
  if filepath =~ '\v/(templates|charts)/.*\.(ya?ml|gotmpl|tpl|txt)$' | return 1 | en
  if filename =~ '\v(helmfile).ya?ml' | return 1 | en
  if getline(1) =~ '^apiVersion:' || getline(2) =~ '^apiVersion:' | return 1 | en
  if !empty(findfile("Chart.yaml", expand('%:p:h').';')) | return 1 | en
  return 0
endfunction

au BufRead,BufNewFile * if s:isHelm() | set ft=helm | en

" Use {{/* */}} as comments
au FileType helm setlocal commentstring={{/*\ %s\ */}}

This should also fix #21