From 2a1bf1443574e668435c82dc0eae27c64a098ac0 Mon Sep 17 00:00:00 2001 From: Aaron Prindle Date: Tue, 30 Nov 2021 16:59:55 -0800 Subject: [PATCH] feat(lint): add end line and col support to lint --- pkg/skaffold/lint/dockerfiles_test.go | 16 ++++--- pkg/skaffold/lint/k8smanifests_test.go | 8 ++-- pkg/skaffold/lint/linters.go | 57 +++++++++++++++++++++---- pkg/skaffold/lint/linters_test.go | 12 ++++-- pkg/skaffold/lint/output.go | 8 ++-- pkg/skaffold/lint/output_test.go | 12 +++--- pkg/skaffold/lint/skaffoldyamls_test.go | 26 +++++++---- pkg/skaffold/lint/types.go | 6 ++- pkg/skaffold/lsp/handler.go | 4 +- 9 files changed, 106 insertions(+), 43 deletions(-) diff --git a/pkg/skaffold/lint/dockerfiles_test.go b/pkg/skaffold/lint/dockerfiles_test.go index e07d61e35f5..65a60b45788 100644 --- a/pkg/skaffold/lint/dockerfiles_test.go +++ b/pkg/skaffold/lint/dockerfiles_test.go @@ -85,9 +85,11 @@ func TestGetDockerfilesLintResults(t *testing.T) { expected: map[string]*[]Result{ "cfg0": { { - Rule: ruleIDToDockerfileRule[DockerfileCopyOver1000Files], - Line: 3, - Column: 1, + Rule: ruleIDToDockerfileRule[DockerfileCopyOver1000Files], + StartLine: 3, + EndLine: 4, + StartColumn: 1, + EndColumn: 0, Explanation: `Found docker 'COPY' command where the source directory "." has over 1000 files. This has the potential ` + `to dramatically slow 'skaffold dev' down as skaffold watches all sources files referenced in dockerfile COPY directives ` + `for changes. If you notice skaffold rebuilding images unnecessarily when non-image-critical files are modified, consider ` + @@ -122,9 +124,11 @@ func TestGetDockerfilesLintResults(t *testing.T) { expected: map[string]*[]Result{ "cfg0": { { - Rule: ruleIDToDockerfileRule[DockerfileCopyContainsGitDir], - Line: 3, - Column: 1, + Rule: ruleIDToDockerfileRule[DockerfileCopyContainsGitDir], + StartLine: 3, + EndLine: 4, + StartColumn: 1, + EndColumn: 0, Explanation: `Found docker 'COPY' command where the source directory "." contains a '.git' directory at .git. This has the potential ` + `to dramatically slow 'skaffold dev' down as skaffold will watch all of the files in the .git directory as skaffold watches all sources ` + `files referenced in dockerfile COPY directives for changes. skaffold will likely rebuild images unnecessarily when non-image-critical ` + diff --git a/pkg/skaffold/lint/k8smanifests_test.go b/pkg/skaffold/lint/k8smanifests_test.go index bfff228ffd0..92c25f4f5d5 100644 --- a/pkg/skaffold/lint/k8smanifests_test.go +++ b/pkg/skaffold/lint/k8smanifests_test.go @@ -82,9 +82,11 @@ func TestGetK8sManifestsLintResults(t *testing.T) { expected: map[string]*[]Result{ "cfg0": { { - Rule: ruleIDToK8sManifestRule[K8sManifestManagedByLabelInUse], - Line: 7, - Column: 5, + Rule: ruleIDToK8sManifestRule[K8sManifestManagedByLabelInUse], + StartLine: 7, + EndLine: 8, + StartColumn: 5, + EndColumn: 0, Explanation: `Found usage of label 'app.kubernetes.io/managed-by'. skaffold overwrites the 'app.kubernetes.io/managed-by' ` + `field to 'app.kubernetes.io/managed-by: skaffold'. and as such is recommended to remove this label`, }, diff --git a/pkg/skaffold/lint/linters.go b/pkg/skaffold/lint/linters.go index 50ed0d369b6..5012fab7b38 100644 --- a/pkg/skaffold/lint/linters.go +++ b/pkg/skaffold/lint/linters.go @@ -68,7 +68,7 @@ func (*DockerfileCommandLinter) Lint(params InputParams, rules *[]Rule) (*[]Resu log.Entry(context.TODO()).Infof("docker command 'copy' match found for source: %s\n", fromTo.From) // TODO(aaron-prindle) modify so that there are input and output params s.t. it is more obvious what fields need to be updated params.DockerCopyCommandInfo = fromTo - appendRuleIfConditionsAndExplanationPopulationsSucceed(params, results, rule, fromTo.StartLine, 1) + appendRuleIfConditionsAndExplanationPopulationsSucceed(params, results, rule, fromTo.StartLine, 1, fromTo.EndLine, 0) } } return results, nil @@ -104,27 +104,53 @@ func (*YamlFieldLinter) Lint(lintInputs InputParams, rules *[]Rule) (*[]Result, // TODO(aaron-prindle) this type of message (last line of file) does not work well in an IDE via the LSP // consider not using this and pinning to somewhere in the yaml or using some type of different messaging (window/showMessage, etc.) line, col := getLastLineAndColOfFile(lintInputs.ConfigFile.Text) - appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, line, col) + // TODO(aaron-prindle) verify this looks correct on an IDE + appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, line, col, line+1, 0) continue } if yamlFilter.FieldMatch != "" { mapnode := node.Field(yamlFilter.FieldMatch) if mapnode != nil { - appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, mapnode.Key.YNode().Line, mapnode.Key.YNode().Column) + ks, err := mapnode.Key.String() + if err != nil { + return nil, err + } + lineLen, endCol := getLinesAndColsOfString(ks) + appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, mapnode.Key.YNode().Line, mapnode.Key.YNode().Column, + mapnode.Key.YNode().Line+lineLen, endCol, + ) } continue } if node.YNode().Kind == yaml.ScalarNode { - appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, node.Document().Line, node.Document().Column) + ns, err := node.String() + if err != nil { + return nil, err + } + lineLen, endCol := getLinesAndColsOfString(ns) + appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, node.Document().Line, node.Document().Column, + node.Document().Line+lineLen, endCol, + ) } for _, n := range node.Content() { - appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, n.Line, n.Column) + ns, err := node.String() + if err != nil { + return nil, err + } + lineLen, endCol := getLinesAndColsOfString(ns) + appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs, results, rule, n.Line, n.Column, + n.Line+lineLen, endCol, + ) } } return results, nil } -func appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs InputParams, results *[]Result, rule Rule, line, col int) { +func appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs InputParams, results *[]Result, rule Rule, startline, startcol, endline, endcol int) { + if startline == endline { + endline++ // this is done to highlight entire line when used w/ an IDE + } + for _, f := range rule.LintConditions { if !f(lintInputs) { // lint condition failed, no rule is trigggered @@ -158,8 +184,10 @@ func appendRuleIfConditionsAndExplanationPopulationsSucceed(lintInputs InputPara Explanation: explanation, AbsFilePath: lintInputs.ConfigFile.AbsPath, RelFilePath: lintInputs.ConfigFile.RelPath, - Line: line, - Column: col, + StartLine: startline, + EndLine: endline, + StartColumn: startcol, + EndColumn: endcol, } *results = append(*results, mr) } @@ -176,3 +204,16 @@ func getLastLineAndColOfFile(input string) (int, int) { } return line, col } + +func getLinesAndColsOfString(str string) (int, int) { + line := 0 + col := 0 + for i := range str { + col++ + if str[i] == '\n' { + line++ + col = 0 + } + } + return line, col +} diff --git a/pkg/skaffold/lint/linters_test.go b/pkg/skaffold/lint/linters_test.go index fe0db202bef..cb8c452de2e 100644 --- a/pkg/skaffold/lint/linters_test.go +++ b/pkg/skaffold/lint/linters_test.go @@ -90,8 +90,10 @@ func TestYamlFieldLinter(t *testing.T) { }, AbsFilePath: "/abs/rel/path", RelFilePath: "rel/path", - Line: 1, - Column: 13, + StartLine: 1, + EndLine: 2, + StartColumn: 13, + EndColumn: 0, Explanation: "test explanation", }, }, @@ -141,8 +143,10 @@ func TestYamlFieldLinter(t *testing.T) { }, AbsFilePath: "/abs/rel/path", RelFilePath: "rel/path", - Line: 23, - Column: 1, + StartLine: 23, + EndLine: 24, + StartColumn: 1, + EndColumn: 0, Explanation: "test explanation", }, }, diff --git a/pkg/skaffold/lint/output.go b/pkg/skaffold/lint/output.go index 5795ec43ced..002e0cf849a 100644 --- a/pkg/skaffold/lint/output.go +++ b/pkg/skaffold/lint/output.go @@ -73,13 +73,13 @@ func generatePlainTextOutput(res *Result) (string, error) { } out := plainTextOutput{ RelFilePath: res.RelFilePath, - LineNumber: res.Line, - ColumnNumber: res.Column, + LineNumber: res.StartLine, + ColumnNumber: res.StartColumn, RuleID: res.Rule.RuleID.String(), Explanation: res.Explanation, RuleType: res.Rule.RuleType.String(), - FlaggedText: strings.Split(string(text), "\n")[res.Line-1], - ColPointerLine: genColPointerLine(res.Column), + FlaggedText: strings.Split(string(text), "\n")[res.StartLine-1], + ColPointerLine: genColPointerLine(res.StartColumn), } // TODO(aaron-prindle) - support different template for multiline matches -> (no ColPointerLine, show multi line match) // if flagged text contains \n character, don't show colpointerline diff --git a/pkg/skaffold/lint/output_test.go b/pkg/skaffold/lint/output_test.go index 3dc5e789889..ad5d13064a7 100644 --- a/pkg/skaffold/lint/output_test.go +++ b/pkg/skaffold/lint/output_test.go @@ -49,8 +49,10 @@ func TestLintOutput(t *testing.T) { Explanation: "test explanation", AbsFilePath: "/abs/rel/path", RelFilePath: "rel/path", - Line: 1, - Column: 1, + StartLine: 1, + EndLine: 2, + StartColumn: 1, + EndColumn: 1, }, }, text: "first column of this line should be flagged in the result [1,1]", @@ -69,13 +71,13 @@ func TestLintOutput(t *testing.T) { }, AbsFilePath: "/abs/rel/path", RelFilePath: "rel/path", - Line: 1, - Column: 1, + StartLine: 1, + StartColumn: 1, Explanation: "test explanation", }, }, text: "first column of this line should be flagged in the result [1,1]", - expected: `[{"Rule":{"RuleID":0,"RuleType":1,"ExplanationTemplate":"","Severity":1,"Filter":null},"AbsFilePath":%#v,"RelFilePath":"rel/path","Explanation":"test explanation","Line":1,"Column":1}]` + "\n", + expected: `[{"Rule":{"RuleID":0,"RuleType":1,"ExplanationTemplate":"","Severity":1,"Filter":null},"AbsFilePath":%#v,"RelFilePath":"rel/path","Explanation":"test explanation","StartLine":1,"EndLine":0,"StartColumn":1,"EndColumn":0}]` + "\n", }, } for _, test := range tests { diff --git a/pkg/skaffold/lint/skaffoldyamls_test.go b/pkg/skaffold/lint/skaffoldyamls_test.go index 81d63a63cb1..42fc0ee7138 100644 --- a/pkg/skaffold/lint/skaffoldyamls_test.go +++ b/pkg/skaffold/lint/skaffoldyamls_test.go @@ -108,16 +108,20 @@ func TestGetSkaffoldYamlsLintResults(t *testing.T) { "cfg0": { { Rule: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate], - Line: 1, - Column: 13, + StartLine: 1, + EndLine: 2, + StartColumn: 13, + EndColumn: 0, Explanation: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate].ExplanationTemplate, }, }, "cfg1": { { Rule: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate], - Line: 1, - Column: 13, + StartLine: 1, + EndLine: 2, + StartColumn: 13, + EndColumn: 0, Explanation: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate].ExplanationTemplate, }, }, @@ -132,8 +136,10 @@ func TestGetSkaffoldYamlsLintResults(t *testing.T) { "cfg0": { { Rule: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate], - Line: 1, - Column: 13, + StartLine: 1, + EndLine: 2, + StartColumn: 13, + EndColumn: 0, Explanation: ruleIDToskaffoldYamlRule[SkaffoldYamlAPIVersionOutOfDate].ExplanationTemplate, }, }, @@ -147,9 +153,11 @@ func TestGetSkaffoldYamlsLintResults(t *testing.T) { expected: map[string]*[]Result{ "cfg0": { { - Rule: ruleIDToskaffoldYamlRule[SkaffoldYamlUseStaticPort], - Line: 14, - Column: 1, + Rule: ruleIDToskaffoldYamlRule[SkaffoldYamlUseStaticPort], + StartLine: 14, + EndLine: 15, + StartColumn: 1, + EndColumn: 0, Explanation: "It is a skaffold best practice to specify a static port (vs skaffold dynamically choosing one) for port forwarding " + "container based resources skaffold deploys. This is helpful because with this the local ports are predictable across dev sessions which " + " makes testing/debugging easier. It is recommended to add the following stanza at the end of your skaffold.yaml for each shown deployed resource:\n" + diff --git a/pkg/skaffold/lint/types.go b/pkg/skaffold/lint/types.go index 67484be46a6..422a335ac47 100644 --- a/pkg/skaffold/lint/types.go +++ b/pkg/skaffold/lint/types.go @@ -59,8 +59,10 @@ type Result struct { AbsFilePath string RelFilePath string Explanation string - Line int - Column int + StartLine int + EndLine int + StartColumn int + EndColumn int } type DockerCommandFilter struct { diff --git a/pkg/skaffold/lsp/handler.go b/pkg/skaffold/lsp/handler.go index a713fdac15b..54897d7a73e 100644 --- a/pkg/skaffold/lsp/handler.go +++ b/pkg/skaffold/lsp/handler.go @@ -197,9 +197,9 @@ func lintFilesAndSendDiagnostics(ctx context.Context, runCtx docker.Config, for _, result := range results { diag := protocol.Diagnostic{ Range: protocol.Range{ - Start: protocol.Position{Line: uint32(result.Line - 1), Character: uint32(result.Column - 1)}, + Start: protocol.Position{Line: uint32(result.StartLine - 1), Character: uint32(result.StartColumn - 1)}, // TODO(aaron-prindle) should implement and pass end range from lint as well (currently a hack and just flags to end of line vs end of flagged text) - End: protocol.Position{Line: uint32(result.Line), Character: 0}, + End: protocol.Position{Line: uint32(result.StartLine), Character: 0}, }, Severity: result.Rule.Severity, Code: result.Rule.RuleID,