-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add Conflictingmarkers #126
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
Changes from 4 commits
14a91e2
9be033a
ebda0ef
ed134ff
90a97b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,138 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
package conflictingmarkers | ||
|
||
import ( | ||
"fmt" | ||
"go/ast" | ||
"strings" | ||
|
||
"golang.org/x/tools/go/analysis" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors" | ||
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" | ||
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector" | ||
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers" | ||
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils" | ||
) | ||
|
||
const name = "conflictingmarkers" | ||
|
||
type analyzer struct { | ||
conflictSets []ConflictSet | ||
} | ||
|
||
func newAnalyzer(cfg *ConflictingMarkersConfig) *analysis.Analyzer { | ||
JoelSpeed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if cfg == nil { | ||
cfg = &ConflictingMarkersConfig{} | ||
} | ||
|
||
// Register markers from configuration | ||
for _, conflictSet := range cfg.Conflicts { | ||
for _, set := range conflictSet.Sets { | ||
for _, markerID := range set { | ||
markers.DefaultRegistry().Register(markerID) | ||
} | ||
} | ||
} | ||
|
||
a := &analyzer{ | ||
conflictSets: cfg.Conflicts, | ||
} | ||
|
||
// Use configured documentation or fall back to default | ||
doc := cfg.Doc | ||
if doc == "" { | ||
doc = "Check that fields do not have conflicting markers from mutually exclusive sets" | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh interesting, hadn't thought about this, when we wrap the analyzer this makes sense doesn't it |
||
|
||
return &analysis.Analyzer{ | ||
Name: name, | ||
Doc: doc, | ||
Run: a.run, | ||
Requires: []*analysis.Analyzer{inspector.Analyzer}, | ||
} | ||
} | ||
|
||
func (a *analyzer) run(pass *analysis.Pass) (any, error) { | ||
inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) | ||
if !ok { | ||
return nil, kalerrors.ErrCouldNotGetInspector | ||
} | ||
|
||
inspect.InspectFields(func(field *ast.Field, stack []ast.Node, _ extractjsontags.FieldTagInfo, markersAccess markers.Markers) { | ||
checkField(pass, field, markersAccess, a.conflictSets) | ||
}) | ||
|
||
return nil, nil //nolint:nilnil | ||
} | ||
|
||
func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, conflictSets []ConflictSet) { | ||
if field == nil || len(field.Names) == 0 { | ||
return | ||
} | ||
|
||
markers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field) | ||
|
||
for _, conflictSet := range conflictSets { | ||
checkConflict(pass, field, markers, conflictSet) | ||
} | ||
} | ||
|
||
func checkConflict(pass *analysis.Pass, field *ast.Field, markers markers.MarkerSet, conflictSet ConflictSet) { | ||
// Track which sets have markers present | ||
conflictingMarkers := make([]sets.Set[string], 0) | ||
|
||
for _, set := range conflictSet.Sets { | ||
foundMarkers := sets.New[string]() | ||
|
||
for _, markerID := range set { | ||
if markers.Has(markerID) { | ||
foundMarkers.Insert(markerID) | ||
} | ||
} | ||
// Only add the set if it has at least one marker | ||
if foundMarkers.Len() > 0 { | ||
conflictingMarkers = append(conflictingMarkers, foundMarkers) | ||
} | ||
} | ||
|
||
// If two or more sets have markers, report the conflict | ||
if len(conflictingMarkers) >= 2 { | ||
reportConflict(pass, field, conflictSet, conflictingMarkers) | ||
} | ||
} | ||
|
||
func reportConflict(pass *analysis.Pass, field *ast.Field, conflictSet ConflictSet, conflictingMarkers []sets.Set[string]) { | ||
// Build a descriptive message showing which sets conflict | ||
setDescriptions := make([]string, 0, len(conflictingMarkers)) | ||
|
||
for _, set := range conflictingMarkers { | ||
markersList := sets.List(set) | ||
setDescriptions = append(setDescriptions, fmt.Sprintf("%v", markersList)) | ||
} | ||
|
||
message := fmt.Sprintf("field %s has conflicting markers: %s: {%s}. %s", | ||
field.Names[0].Name, | ||
conflictSet.Name, | ||
strings.Join(setDescriptions, ", "), | ||
conflictSet.Description) | ||
|
||
pass.Report(analysis.Diagnostic{ | ||
Pos: field.Pos(), | ||
Message: message, | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
package conflictingmarkers_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"golang.org/x/tools/go/analysis/analysistest" | ||
"sigs.k8s.io/kube-api-linter/pkg/analysis/conflictingmarkers" | ||
) | ||
|
||
func TestConflictingMarkersAnalyzer(t *testing.T) { | ||
testdata := analysistest.TestData() | ||
|
||
config := &conflictingmarkers.ConflictingMarkersConfig{ | ||
Conflicts: []conflictingmarkers.ConflictSet{ | ||
{ | ||
Name: "test_conflict", | ||
Sets: [][]string{{"marker1", "marker2"}, {"marker3", "marker4"}}, | ||
Description: "Test markers conflict with each other", | ||
}, | ||
{ | ||
Name: "three_way_conflict", | ||
Sets: [][]string{{"marker5", "marker6"}, {"marker7", "marker8"}, {"marker9", "marker10"}}, | ||
Description: "Three-way conflict between marker sets", | ||
}, | ||
}, | ||
} | ||
|
||
initializer := conflictingmarkers.Initializer() | ||
|
||
analyzer, err := initializer.Init(config) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
analysistest.Run(t, testdata, analyzer, "a") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
package conflictingmarkers | ||
|
||
// ConflictingMarkersConfig contains the configuration for the conflictingmarkers linter. | ||
type ConflictingMarkersConfig struct { | ||
// Doc is the documentation string for the analyzer. | ||
// If not provided, a default description will be used. | ||
Doc string `json:"doc"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this leaking implementation details into user-facing configuration options? For example, as an end user I would now be able to configure this in a way that the conflicting markers analyzer uses a custom doc string. I would generally think this is something that we don't want to allow users to change in configuration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where does this doc string actually show up? Does it matter if analyzers wrapping this one don't have the option to change it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it would show under the output of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It won't be reflected by |
||
// Conflicts allows users to define sets of conflicting markers. | ||
// Each entry defines a conflict between multiple sets of markers. | ||
Conflicts []ConflictSet `json:"conflicts"` | ||
} | ||
|
||
// ConflictSet represents a conflict between multiple sets of markers. | ||
// Markers within each set are mutually exclusive with markers in all other sets. | ||
// The linter will emit a diagnostic when a field has markers from two or more sets. | ||
type ConflictSet struct { | ||
// Name is a human-readable name for this conflict set. | ||
JoelSpeed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// This name will appear in diagnostic messages to identify the type of conflict. | ||
Name string `json:"name"` | ||
// Sets contains the sets of markers that are mutually exclusive with each other. | ||
// Each set is a slice of marker identifiers. | ||
// The linter will emit a diagnostic when a field has markers from two or more sets. | ||
Sets [][]string `json:"sets"` | ||
// Description provides a description of why these markers conflict. | ||
JoelSpeed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// The linter will include this description in the diagnostic message when a conflict is detected. | ||
Description string `json:"description"` | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
Copyright 2025 The Kubernetes Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package conflictingmarkers_test | ||
|
||
import ( | ||
"testing" | ||
|
||
. "github.com/onsi/ginkgo/v2" | ||
. "github.com/onsi/gomega" | ||
) | ||
|
||
func TestConflictingMarkers(t *testing.T) { | ||
RegisterFailHandler(Fail) | ||
RunSpecs(t, "conflictingmarkers") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry is now in the list twice, and it appears we have the documentation twice too?