Skip to content

Commit

Permalink
feat(lint): add end line and col support to lint
Browse files Browse the repository at this point in the history
  • Loading branch information
@aaron-prindle
aaron-prindle committed Dec 8, 2021
1 parent 4b0579c commit 2a1bf14
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 43 deletions.
16 changes: 10 additions & 6 deletions pkg/skaffold/lint/dockerfiles_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ` +
Expand Down Expand Up @@ -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 ` +
Expand Down
8 changes: 5 additions & 3 deletions pkg/skaffold/lint/k8smanifests_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
},
Expand Down
57 changes: 49 additions & 8 deletions pkg/skaffold/lint/linters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand All @@ -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
}
12 changes: 8 additions & 4 deletions pkg/skaffold/lint/linters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
Expand Down Expand Up @@ -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",
},
},
Expand Down
8 changes: 4 additions & 4 deletions pkg/skaffold/lint/output.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions pkg/skaffold/lint/output_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]",
Expand All @@ -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 {
Expand Down
26 changes: 17 additions & 9 deletions pkg/skaffold/lint/skaffoldyamls_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
Expand All @@ -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,
},
},
Expand All @@ -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" +
Expand Down
6 changes: 4 additions & 2 deletions pkg/skaffold/lint/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/lsp/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 2a1bf14

Please sign in to comment.