From 736cc78b5ff8629ef77fb88bf23bcdf2926e9a44 Mon Sep 17 00:00:00 2001 From: qvalentin Date: Sat, 11 May 2024 14:41:06 +0200 Subject: [PATCH] feat(lint): rework diagnostics --- internal/adapter/yamlls/documentSync.go | 2 +- internal/handler/diagnostics.go | 17 +++ internal/handler/text_document.go | 14 +- internal/lsp/lint.go | 163 ++++++++++++------------ internal/lsp/lint_test.go | 53 +++++++- internal/util/values.go | 6 +- 6 files changed, 166 insertions(+), 89 deletions(-) create mode 100644 internal/handler/diagnostics.go diff --git a/internal/adapter/yamlls/documentSync.go b/internal/adapter/yamlls/documentSync.go index 1d566b58..9c10e054 100644 --- a/internal/adapter/yamlls/documentSync.go +++ b/internal/adapter/yamlls/documentSync.go @@ -91,7 +91,7 @@ func (yamllsConnector Connector) DocumentDidChangeFullSync(doc *lsplocal.Documen return } - logger.Println("Sending DocumentDidChange with full sync, current content:", doc.Content) + logger.Debug("Sending DocumentDidChange with full sync, current content:", doc.Content) trimmedText := lsplocal.TrimTemplate(doc.Ast.Copy(), doc.Content) params.ContentChanges = []lsp.TextDocumentContentChangeEvent{ diff --git a/internal/handler/diagnostics.go b/internal/handler/diagnostics.go new file mode 100644 index 00000000..3c7f8598 --- /dev/null +++ b/internal/handler/diagnostics.go @@ -0,0 +1,17 @@ +package handler + +import ( + "context" + + lsp "go.lsp.dev/protocol" +) + +func (h *langHandler) publishDiagnostics(ctx context.Context, notifications []lsp.PublishDiagnosticsParams) { + for _, notification := range notifications { + logger.Debug("Publishing diagnostics notification", notification) + err := h.client.PublishDiagnostics(ctx, ¬ification) + if err != nil { + logger.Error("Error publishing diagnostics ", err) + } + } +} diff --git a/internal/handler/text_document.go b/internal/handler/text_document.go index d8c75a8e..a2ac6b2a 100644 --- a/internal/handler/text_document.go +++ b/internal/handler/text_document.go @@ -28,12 +28,14 @@ func (h *langHandler) DidOpen(ctx context.Context, params *lsp.DidOpenTextDocume if err != nil { logger.Error("Error getting chart info for file", doc.URI, err) } - notification := lsplocal.GetDiagnosticsNotification(chart, doc) + notifications := lsplocal.GetDiagnosticsNotifications(chart, doc) - return h.client.PublishDiagnostics(ctx, notification) + defer h.publishDiagnostics(ctx, notifications) + + return nil } -func (h *langHandler) DidClose(ctx context.Context, params *lsp.DidCloseTextDocumentParams) (err error) { +func (h *langHandler) DidClose(_ context.Context, _ *lsp.DidCloseTextDocumentParams) (err error) { return nil } @@ -48,9 +50,11 @@ func (h *langHandler) DidSave(ctx context.Context, params *lsp.DidSaveTextDocume } h.yamllsConnector.DocumentDidSave(doc, *params) - notification := lsplocal.GetDiagnosticsNotification(chart, doc) + notifications := lsplocal.GetDiagnosticsNotifications(chart, doc) - return h.client.PublishDiagnostics(ctx, notification) + defer h.publishDiagnostics(ctx, notifications) + + return nil } func (h *langHandler) DidChange(ctx context.Context, params *lsp.DidChangeTextDocumentParams) (err error) { diff --git a/internal/lsp/lint.go b/internal/lsp/lint.go index 8969d068..c449286a 100644 --- a/internal/lsp/lint.go +++ b/internal/lsp/lint.go @@ -2,6 +2,7 @@ package lsp import ( "fmt" + "path/filepath" "strconv" "strings" @@ -13,126 +14,130 @@ import ( lsp "go.lsp.dev/protocol" "go.lsp.dev/uri" "helm.sh/helm/v3/pkg/action" - // "helm.sh/helm/v3/pkg/lint/rules" + "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/lint/support" ) var logger = log.GetLogger() -func GetDiagnosticsNotification(chart *charts.Chart, doc *Document) *lsp.PublishDiagnosticsParams { +func GetDiagnosticsNotifications(chart *charts.Chart, doc *Document) []lsp.PublishDiagnosticsParams { vals := chart.ValuesFiles.MainValuesFile.Values if chart.ValuesFiles.OverlayValuesFile != nil { vals = chartutil.CoalesceTables(chart.ValuesFiles.OverlayValuesFile.Values, chart.ValuesFiles.MainValuesFile.Values) } - diagnostics := GetDiagnostics(doc.URI, vals) - doc.DiagnosticsCache.HelmDiagnostics = diagnostics - - return &lsp.PublishDiagnosticsParams{ - URI: doc.URI, - Diagnostics: doc.DiagnosticsCache.GetMergedDiagnostics(), + diagnostics := GetDiagnostics(chart.RootURI, vals) + + // Update the diagnostics cache only for the currently opened document + // as it will also get diagnostics from yamlls + // if currentDocDiagnostics is empty it means that all issues in that file have been fixed + // we need to send this to the client + currentDocDiagnostics := diagnostics[string(doc.URI.Filename())] + doc.DiagnosticsCache.HelmDiagnostics = currentDocDiagnostics + diagnostics[string(doc.URI.Filename())] = doc.DiagnosticsCache.GetMergedDiagnostics() + + result := []lsp.PublishDiagnosticsParams{} + + for diagnosticsURI, diagnostics := range diagnostics { + result = append(result, + lsp.PublishDiagnosticsParams{ + URI: uri.File(diagnosticsURI), + Diagnostics: diagnostics, + }, + ) } + + return result } -// GetDiagnostics will run helm linter against the currect document URI using the given values +// GetDiagnostics will run helm linter against the chart root URI using the given values // and converts the helm.support.Message to lsp.Diagnostics -func GetDiagnostics(uri uri.URI, vals chartutil.Values) []lsp.Diagnostic { - var ( - filename = uri.Filename() - paths = strings.Split(filename, "/") - dir = strings.Join(paths, "/") - diagnostics = make([]lsp.Diagnostic, 0) - ) - - pathfile := "" - - for i, p := range paths { - if p == "templates" { - dir = strings.Join(paths[0:i], "/") - pathfile = strings.Join(paths[i:], "/") - } - } +func GetDiagnostics(rootURI uri.URI, vals chartutil.Values) map[string][]lsp.Diagnostic { + diagnostics := map[string][]lsp.Diagnostic{} client := action.NewLint() - result := client.Run([]string{dir}, vals) - logger.Println(fmt.Sprintf("helm lint: result for file %s : %s", uri, result.Messages)) + result := client.Run([]string{rootURI.Filename()}, vals) for _, msg := range result.Messages { - d, filename, err := GetDiagnosticFromLinterErr(msg) - if err != nil { - continue - } - if filename != pathfile { - continue + d, relativeFilePath, _ := GetDiagnosticFromLinterErr(msg) + absoluteFilePath := filepath.Join(rootURI.Filename(), string(relativeFilePath)) + if d != nil { + diagnostics[absoluteFilePath] = append(diagnostics[absoluteFilePath], *d) } - diagnostics = append(diagnostics, *d) } - logger.Println(fmt.Sprintf("helm lint: result for file %s : %v", uri, diagnostics)) + logger.Println(fmt.Sprintf("helm lint: result for chart %s : %v", rootURI.Filename(), diagnostics)) return diagnostics } func GetDiagnosticFromLinterErr(supMsg support.Message) (*lsp.Diagnostic, string, error) { - var ( - err error - msg string - line = 1 - severity lsp.DiagnosticSeverity - filename = getFilePathFromLinterErr(supMsg) - ) - - switch supMsg.Severity { - case support.ErrorSev: - - severity = lsp.DiagnosticSeverityError - - // if superr, ok := supMsg.Err.(*rules.YAMLToJSONParseError); ok { - - // line = superr.Line - // msg = superr.Error() - // - // } else { + severity := parseSeverity(supMsg) - fileLine := util.BetweenStrings(supMsg.Error(), "(", ")") - fileLineArr := strings.Split(fileLine, ":") - if len(fileLineArr) < 2 { - return nil, filename, errors.Errorf("linter Err contains no position information") - } - lineStr := fileLineArr[1] - line, err = strconv.Atoi(lineStr) + if strings.HasPrefix(supMsg.Path, "templates") { + message, err := parseTemplatesMessage(supMsg, severity) + path := getFilePathFromLinterErr(supMsg) if err != nil { - return nil, filename, err + return nil, "", err } - msgStr := util.AfterStrings(supMsg.Error(), "):") - msg = strings.TrimSpace(msgStr) - - // } - - case support.WarningSev: - - // severity = lsp.DiagnosticSeverityWarning - // if err, ok := supMsg.Err.(*rules.MetadataError); ok { - // line = 1 - // msg = err.Details().Error() - // } + return &message, path, nil + } - case support.InfoSev: + message := string(supMsg.Err.Error()) + // NOTE: The diagnostics may not be shown correctly in the Chart.yaml file in neovim + // because the lsp is not active for that file + if supMsg.Path == "Chart.yaml" || strings.Contains(message, "chart metadata") { + return &lsp.Diagnostic{ + Severity: severity, + Source: "Helm lint", + Message: message, + }, "Chart.yaml", nil + } - severity = lsp.DiagnosticSeverityInformation - msg = supMsg.Err.Error() + return nil, "", nil +} +func parseTemplatesMessage(supMsg support.Message, severity lsp.DiagnosticSeverity) (lsp.Diagnostic, error) { + var ( + err error + line int + fileLine = util.BetweenStrings(supMsg.Error(), "(", ")") + fileLineArr = strings.Split(fileLine, ":") + ) + if len(fileLineArr) < 2 { + return lsp.Diagnostic{}, errors.Errorf("linter Err contains no position information") + } + lineStr := fileLineArr[1] + line, err = strconv.Atoi(lineStr) + if err != nil { + return lsp.Diagnostic{}, err } + msgStr := util.AfterStrings(supMsg.Error(), "):") + msg := strings.TrimSpace(msgStr) - return &lsp.Diagnostic{ + return lsp.Diagnostic{ Range: lsp.Range{ Start: lsp.Position{Line: uint32(line - 1)}, End: lsp.Position{Line: uint32(line - 1)}, }, Severity: severity, + Source: "Helm lint", Message: msg, - }, filename, nil + }, nil +} + +func parseSeverity(supMsg support.Message) lsp.DiagnosticSeverity { + var severity lsp.DiagnosticSeverity + switch supMsg.Severity { + case support.ErrorSev: + severity = lsp.DiagnosticSeverityError + case support.WarningSev: + severity = lsp.DiagnosticSeverityWarning + case support.InfoSev: + severity = lsp.DiagnosticSeverityInformation + } + return severity } func getFilePathFromLinterErr(msg support.Message) string { diff --git a/internal/lsp/lint_test.go b/internal/lsp/lint_test.go index 7db62846..ba8968ff 100644 --- a/internal/lsp/lint_test.go +++ b/internal/lsp/lint_test.go @@ -3,12 +3,63 @@ package lsp import ( "testing" + "github.com/mrjosh/helm-ls/internal/charts" "github.com/stretchr/testify/assert" "go.lsp.dev/uri" "helm.sh/helm/v3/pkg/chartutil" ) func TestLint(t *testing.T) { - diagnostics := GetDiagnostics(uri.File("../../testdata/example/templates/lint.yaml"), chartutil.Values{}) + diagnostics := GetDiagnostics(uri.File("../../testdata/example"), chartutil.Values{}) assert.NotEmpty(t, diagnostics) + assert.Len(t, diagnostics, 2) + assert.Len(t, diagnostics[uri.File("../../testdata/example/Chart.yaml").Filename()], 1) +} + +func TestLintNotifications(t *testing.T) { + chart := charts.Chart{ + RootURI: uri.File("../../testdata/example"), + ValuesFiles: &charts.ValuesFiles{ + MainValuesFile: &charts.ValuesFile{}, + OverlayValuesFile: &charts.ValuesFile{}, + AdditionalValuesFiles: []*charts.ValuesFile{}, + }, + } + diagnostics := GetDiagnosticsNotifications(&chart, &Document{URI: uri.File("../../testdata/example/templates/deployment-no-templates.yaml")}) + assert.NotEmpty(t, diagnostics) + assert.Len(t, diagnostics, 3) + + uris := []string{} + for _, notification := range diagnostics { + uris = append(uris, notification.URI.Filename()) + } + assert.Contains(t, uris, uri.File("../../testdata/example/templates/deployment-no-templates.yaml").Filename()) + for _, notification := range diagnostics { + if notification.URI.Filename() == uri.File("../../testdata/example/templates/deployment-no-templates.yaml").Filename() { + assert.Empty(t, notification.Diagnostics) + } + } +} + +func TestLintNotificationsIncludesEmptyDiagnosticsForFixedIssues(t *testing.T) { + chart := charts.Chart{ + RootURI: uri.File("../../testdata/example"), + ValuesFiles: &charts.ValuesFiles{ + MainValuesFile: &charts.ValuesFile{}, + OverlayValuesFile: &charts.ValuesFile{}, + AdditionalValuesFiles: []*charts.ValuesFile{}, + }, + } + diagnostics := GetDiagnosticsNotifications(&chart, &Document{URI: uri.File("../../testdata/example/templates/deployment-no-templates.yaml")}) + + uris := []string{} + for _, notification := range diagnostics { + uris = append(uris, notification.URI.Filename()) + } + assert.Contains(t, uris, uri.File("../../testdata/example/templates/deployment-no-templates.yaml").Filename()) + for _, notification := range diagnostics { + if notification.URI.Filename() == uri.File("../../testdata/example/templates/deployment-no-templates.yaml").Filename() { + assert.Empty(t, notification.Diagnostics) + } + } } diff --git a/internal/util/values.go b/internal/util/values.go index 7be97f06..d13496cb 100644 --- a/internal/util/values.go +++ b/internal/util/values.go @@ -66,7 +66,8 @@ func valuesLookup(values chartutil.Values, splittedVar []string) (chartutil.Valu // PathValue takes a path that traverses a YAML structure and returns the value at the end of that path. // The path starts at the root of the YAML structure and is comprised of YAML keys separated by periods. -// Given the following YAML data the value at path "chapter.one.title" is "Loomings". +// Given the following YAML data the value at path "chapter.one.title" is "Loomings". The path can also +// include array indexes as in "chapters[].title" which will use the first element of the array. // // chapter: // one: @@ -152,8 +153,7 @@ func builCompletionItem(value interface{}, variable string) lsp.CompletionItem { } func FormatToYAML(field reflect.Value, fieldName string) string { - x := field.Kind() - switch x { + switch field.Kind() { case reflect.String: return field.String() case reflect.Map: