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

docs(README): add combined injections #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

baptman21
Copy link
Collaborator

Following #4 , we only need injections with the combined option to get a better result, this should fix most render issues, maybe even the ones in #10 but this needs to be checked.

In the mean time this injection gives a better render.

@baptman21
Copy link
Collaborator Author

I also added the injections file, we need to check if adding it in the queries directory makes it usable without manually adding it.

@qvalentin
Copy link
Collaborator

qvalentin commented Dec 6, 2023

I think I found a case where this does not work correctly:

{{ if .Values }}
        - name: {{ .Chart.Name }}-{{ $variable }}
          image: "test"
{{- end }}    

the yaml after $variable is not rendered correctly.

Can we fix this?

Edit: I guess i know, why this is not working, correct me if I'm wrong.
The injected yaml looks like this, which is not highlighted correctly by the yaml grammar.

        - name:                  -               
          image: "test"

With the combined injections, this will break the yaml highlighting for the rest of the file, the old option would only break one line.

Other than that I can confirm that this fixes the issues described in #3, so #10 would not be needed anymore.

@kmoschcau
Copy link

kmoschcau commented Jan 4, 2024

@qvalentin I just tinkered around with this myself and it can be fixed by quoting the value like so:

- name: "{{ .Chart.Name }}-{{ $variable }}"

That should be good practice anyway and won't risk breaking the generated yaml, if the template values evaluate to whitespace.

@qvalentin
Copy link
Collaborator

I'm also fine with merging this, I just wanted to point out what I noticed.

@qvalentin
Copy link
Collaborator

qvalentin commented Jan 4, 2024

Did I just find another case? I can't really explain this one.

spec:
  triggers:
  - type: prometheus
    metadata:
      query: sum(rate(http_requests_total{deployment="my-deployment"}[2m]))
      quer: test
      broken: highlight

@cabrinha
Copy link

Did I just find another case? I can't really explain this one.

spec:
  triggers:
  - type: prometheus
    metadata:
      query: sum(rate(http_requests_total{deployment="my-deployment"}[2m]))
      quer: test
      broken: highlight

the query should be wrapped in single quotes.

@cabrinha
Copy link

I think I found a case where this does not work correctly:

{{ if .Values }}
        - name: {{ .Chart.Name }}-{{ $variable }}
          image: "test"
{{- end }}    

the yaml after $variable is not rendered correctly.

Can we fix this?

Edit: I guess i know, why this is not working, correct me if I'm wrong. The injected yaml looks like this, which is not highlighted correctly by the yaml grammar.

        - name:                  -               
          image: "test"

With the combined injections, this will break the yaml highlighting for the rest of the file, the old option would only break one line.

Other than that I can confirm that this fixes the issues described in #3, so #10 would not be needed anymore.

This is expected and not an issue.

Screenshot 2024-05-16 at 3 52 38 PM

@qvalentin
Copy link
Collaborator

@baptman21 lets merge this, it is also already included in https://github.com/nvim-treesitter/nvim-treesitter like this already.

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

Successfully merging this pull request may close these issues.

4 participants