Skip to content

Commit 52a16ef

Browse files
yongruilinyongruilin
committed
Address comment: enhance error messages with struct and field names; standardize type values
1 parent 78ae12a commit 52a16ef

File tree

7 files changed

+100
-101
lines changed

7 files changed

+100
-101
lines changed

‎docs/linters.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ lintersConfig:
116116
dependents:
117117
- "k8s:listType"
118118
- identifier: "example:any"
119-
type: "any"
119+
type: "Any"
120120
dependents:
121121
- "dep1"
122122
- "dep2"

‎pkg/analysis/dependenttags/analyzer.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) {
6565
return nil, kalerrors.ErrCouldNotGetInspector
6666
}
6767

68-
inspect.InspectFields(func(field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markers.Markers) {
68+
inspect.InspectFields(func(field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markers.Markers, qualifiedFieldName string) {
6969
if field.Doc == nil {
7070
return
7171
}
@@ -98,7 +98,8 @@ func handleAll(pass *analysis.Pass, field *ast.Field, rule Rule, fieldMarkers ma
9898
}
9999

100100
if len(missing) > 0 {
101-
pass.Reportf(field.Pos(), "field with marker +%s is missing required marker(s): %s", rule.Identifier, strings.Join(missing, ", "))
101+
structName, fieldName := utils.GetStructAndFieldName(pass, field)
102+
pass.Reportf(field.Pos(), "field %s.%s with marker +%s is missing required marker(s): %s", structName, fieldName, rule.Identifier, strings.Join(missing, ", "))
102103
}
103104
}
104105

@@ -118,6 +119,7 @@ func handleAny(pass *analysis.Pass, field *ast.Field, rule Rule, fieldMarkers ma
118119
dependents[i] = fmt.Sprintf("+%s", d)
119120
}
120121

121-
pass.Reportf(field.Pos(), "field with marker +%s requires at least one of the following markers, but none were found: %s", rule.Identifier, strings.Join(dependents, ", "))
122+
structName, fieldName := utils.GetStructAndFieldName(pass, field)
123+
pass.Reportf(field.Pos(), "field %s.%s with marker +%s requires at least one of the following markers, but none were found: %s", structName, fieldName, rule.Identifier, strings.Join(dependents, ", "))
122124
}
123125
}

‎pkg/analysis/dependenttags/config.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ type DependencyType string
2121

2222
const (
2323
// DependencyTypeAll indicates that all dependent markers are required.
24-
DependencyTypeAll DependencyType = "all"
24+
DependencyTypeAll DependencyType = "All"
2525
// DependencyTypeAny indicates that at least one of the dependent markers is required.
26-
DependencyTypeAny DependencyType = "any"
26+
DependencyTypeAny DependencyType = "Any"
2727
)
2828

2929
// Config defines the configuration for the dependenttags linter.

‎pkg/analysis/dependenttags/initializer.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package dependenttags
1818

1919
import (
20+
"fmt"
21+
2022
"golang.org/x/tools/go/analysis"
2123
"k8s.io/apimachinery/pkg/util/validation/field"
2224
"sigs.k8s.io/kube-api-linter/pkg/analysis/initializer"
@@ -73,13 +75,13 @@ func validateConfig(cfg *Config, fldPath *field.Path) field.ErrorList {
7375
}
7476

7577
if rule.Type == "" {
76-
errs = append(errs, field.Required(rulesPath.Index(i).Child("type"), "type must be explicitly set to 'all' or 'any'"))
78+
errs = append(errs, field.Required(rulesPath.Index(i).Child("type"), fmt.Sprintf("type must be explicitly set to '%s' or '%s'", DependencyTypeAll, DependencyTypeAny)))
7779
} else {
7880
switch rule.Type {
7981
case DependencyTypeAll, DependencyTypeAny:
8082
// valid
8183
default:
82-
errs = append(errs, field.Invalid(rulesPath.Index(i).Child("type"), rule.Type, "type must be 'all' or 'any'"))
84+
errs = append(errs, field.Invalid(rulesPath.Index(i).Child("type"), rule.Type, fmt.Sprintf("type must be '%s' or '%s'", DependencyTypeAll, DependencyTypeAny)))
8385
}
8486
}
8587
}

‎pkg/analysis/dependenttags/initializer_test.go‎

Lines changed: 76 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -17,107 +17,94 @@ limitations under the License.
1717
package dependenttags_test
1818

1919
import (
20-
"testing"
20+
"fmt"
2121

22+
. "github.com/onsi/ginkgo/v2"
23+
. "github.com/onsi/gomega"
2224
"k8s.io/apimachinery/pkg/util/validation/field"
2325
"sigs.k8s.io/kube-api-linter/pkg/analysis/dependenttags"
2426
)
2527

26-
func TestInitializer(t *testing.T) {
27-
cases := []struct {
28-
name string
29-
cfg *dependenttags.Config
30-
expectErr bool
31-
}{
32-
{
33-
name: "valid config",
34-
cfg: &dependenttags.Config{
35-
Rules: []dependenttags.Rule{
36-
{
37-
Identifier: "k8s:unionMember",
38-
Type: dependenttags.DependencyTypeAll,
39-
Dependents: []string{"k8s:optional"},
28+
var _ = Describe("dependenttags initializer", func() {
29+
Context("config validation", func() {
30+
type testCase struct {
31+
config dependenttags.Config
32+
expectedErr string
33+
}
34+
35+
DescribeTable("should validate the provided config", func(in testCase) {
36+
ci := dependenttags.Initializer()
37+
38+
errs := ci.ValidateConfig(&in.config, field.NewPath("dependenttags"))
39+
if len(in.expectedErr) > 0 {
40+
Expect(errs.ToAggregate()).To(MatchError(in.expectedErr))
41+
} else {
42+
Expect(errs).To(HaveLen(0), "No errors were expected")
43+
}
44+
},
45+
Entry("with a valid config", testCase{
46+
config: dependenttags.Config{
47+
Rules: []dependenttags.Rule{
48+
{
49+
Identifier: "k8s:unionMember",
50+
Type: dependenttags.DependencyTypeAll,
51+
Dependents: []string{"k8s:optional"},
52+
},
4053
},
4154
},
42-
},
43-
expectErr: false,
44-
},
45-
{
46-
name: "missing type",
47-
cfg: &dependenttags.Config{
48-
Rules: []dependenttags.Rule{
49-
{
50-
Identifier: "k8s:unionMember",
51-
Dependents: []string{"k8s:optional"},
55+
expectedErr: "",
56+
}),
57+
Entry("with missing type", testCase{
58+
config: dependenttags.Config{
59+
Rules: []dependenttags.Rule{
60+
{
61+
Identifier: "k8s:unionMember",
62+
Dependents: []string{"k8s:optional"},
63+
},
5264
},
5365
},
54-
},
55-
expectErr: true,
56-
},
57-
{
58-
name: "empty rules",
59-
cfg: &dependenttags.Config{
60-
Rules: []dependenttags.Rule{},
61-
},
62-
expectErr: true,
63-
},
64-
{
65-
name: "missing identifier",
66-
cfg: &dependenttags.Config{
67-
Rules: []dependenttags.Rule{
68-
{
69-
Type: dependenttags.DependencyTypeAll,
70-
Dependents: []string{"k8s:optional"},
66+
expectedErr: fmt.Sprintf("dependenttags.rules[0].type: Required value: type must be explicitly set to '%s' or '%s'", dependenttags.DependencyTypeAll, dependenttags.DependencyTypeAny),
67+
}),
68+
Entry("with empty rules", testCase{
69+
config: dependenttags.Config{
70+
Rules: []dependenttags.Rule{},
71+
},
72+
expectedErr: "dependenttags.rules: Invalid value: []dependenttags.Rule{}: rules cannot be empty",
73+
}),
74+
Entry("with missing identifier", testCase{
75+
config: dependenttags.Config{
76+
Rules: []dependenttags.Rule{
77+
{
78+
Type: dependenttags.DependencyTypeAll,
79+
Dependents: []string{"k8s:optional"},
80+
},
7181
},
7282
},
73-
},
74-
expectErr: true,
75-
},
76-
{
77-
name: "missing dependents",
78-
cfg: &dependenttags.Config{
79-
Rules: []dependenttags.Rule{
80-
{
81-
Identifier: "k8s:unionMember",
82-
Type: dependenttags.DependencyTypeAll,
83+
expectedErr: "dependenttags.rules[0].identifier: Invalid value: \"\": identifier marker cannot be empty",
84+
}),
85+
Entry("with missing dependents", testCase{
86+
config: dependenttags.Config{
87+
Rules: []dependenttags.Rule{
88+
{
89+
Identifier: "k8s:unionMember",
90+
Type: dependenttags.DependencyTypeAll,
91+
},
8392
},
8493
},
85-
},
86-
expectErr: true,
87-
},
88-
{
89-
name: "invalid type",
90-
cfg: &dependenttags.Config{
91-
Rules: []dependenttags.Rule{
92-
{
93-
Identifier: "k8s:unionMember",
94-
Type: "invalid",
95-
Dependents: []string{"k8s:optional"},
94+
expectedErr: "dependenttags.rules[0].dependents: Invalid value: []string(nil): dependents list cannot be empty",
95+
}),
96+
Entry("with invalid type", testCase{
97+
config: dependenttags.Config{
98+
Rules: []dependenttags.Rule{
99+
{
100+
Identifier: "k8s:unionMember",
101+
Type: "invalid",
102+
Dependents: []string{"k8s:optional"},
103+
},
96104
},
97105
},
98-
},
99-
expectErr: true,
100-
},
101-
}
102-
103-
for _, tc := range cases {
104-
t.Run(tc.name, func(t *testing.T) {
105-
initializer := dependenttags.Initializer()
106-
errs := initializer.ValidateConfig(tc.cfg, field.NewPath(""))
107-
108-
if tc.expectErr && len(errs) == 0 {
109-
t.Errorf("expected validation errors, but got none")
110-
}
111-
112-
if !tc.expectErr && len(errs) > 0 {
113-
t.Errorf("unexpected validation errors: %v", errs)
114-
}
115-
116-
if !tc.expectErr {
117-
if _, err := initializer.Init(tc.cfg); err != nil {
118-
t.Errorf("unexpected error initializing analyzer: %v", err)
119-
}
120-
}
121-
})
122-
}
123-
}
106+
expectedErr: fmt.Sprintf(`dependenttags.rules[0].type: Invalid value: "invalid": type must be '%s' or '%s'`, dependenttags.DependencyTypeAll, dependenttags.DependencyTypeAny),
107+
}),
108+
)
109+
})
110+
})

‎pkg/analysis/dependenttags/testdata/src/a/a.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ package a
1818

1919
type Union struct {
2020
// +k8s:unionMember
21-
InvalidMember string // want "field with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"
21+
InvalidMember string // want "field Union.InvalidMember with marker \\+k8s:unionMember is missing required marker\\(s\\): \\+k8s:optional"
2222

2323
// +k8s:unionMember
2424
// +k8s:optional
@@ -27,7 +27,7 @@ type Union struct {
2727

2828
type List struct {
2929
// +listType
30-
InvalidList []string // want "field with marker \\+listType is missing required marker\\(s\\): \\+k8s:listType"
30+
InvalidList []string // want "field List.InvalidList with marker \\+listType is missing required marker\\(s\\): \\+k8s:listType"
3131

3232
// +listType
3333
// +k8s:listType
@@ -36,7 +36,7 @@ type List struct {
3636

3737
type AnyOf struct {
3838
// +example:any
39-
InvalidAny string // want "field with marker \\+example:any requires at least one of the following markers, but none were found: \\+dep1, \\+dep2"
39+
InvalidAny string // want "field AnyOf.InvalidAny with marker \\+example:any requires at least one of the following markers, but none were found: \\+dep1, \\+dep2"
4040

4141
// +example:any
4242
// +dep1
@@ -52,7 +52,7 @@ type MyString string
5252

5353
type TypeAlias struct {
5454
// +example:any
55-
InvalidAlias string // want "field with marker \\+example:any requires at least one of the following markers, but none were found: \\+dep1, \\+dep2"
55+
InvalidAlias string // want "field TypeAlias.InvalidAlias with marker \\+example:any requires at least one of the following markers, but none were found: \\+dep1, \\+dep2"
5656

5757
// +example:any
5858
ValidAlias MyString

‎pkg/analysis/utils/utils.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,3 +491,11 @@ func getFieldTypeName(field *ast.Field) string {
491491

492492
return ""
493493
}
494+
495+
// GetStructAndFieldName returns the name of the struct and the field name for a given field.
496+
func GetStructAndFieldName(pass *analysis.Pass, field *ast.Field) (string, string) {
497+
structName := GetStructNameForField(pass, field)
498+
fieldName := FieldName(field)
499+
500+
return structName, fieldName
501+
}

0 commit comments

Comments
 (0)