Added built-in api group list for CRD check#136
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new utility function in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apigroups/builtin.go (1)
5-6: Consider clarifying the comment to reflect the actual scope of included groups.The comment states "in-tree Kubernetes API groups" but the map also includes CoreOS groups (
operators.coreos.com,packages.operators.coreos.com,monitoring.coreos.com) which are not strictly in-tree Kubernetes. A more accurate description would help maintainers understand what belongs here.📝 Suggested comment clarification
-// builtinK8sAPIGroups lists in-tree Kubernetes API groups that are not backed by -// user-defined CRDs. Extend when new first-party groups appear. +// builtinK8sAPIGroups lists in-tree Kubernetes API groups plus well-known +// platform groups (e.g., CoreOS/OLM) that are not backed by user-defined CRDs. +// Extend when new first-party or platform groups appear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apigroups/builtin.go` around lines 5 - 6, The comment above the builtinK8sAPIGroups map is misleading because it says "in-tree Kubernetes API groups" while the map (builtinK8sAPIGroups) includes non-in-tree groups such as operators.coreos.com, packages.operators.coreos.com, and monitoring.coreos.com; update the comment in apigroups/builtin.go to accurately describe the map's scope (e.g., "first-party and commonly bundled API groups not backed by user-defined CRDs" or note inclusion of CoreOS/operator groups) so maintainers know why those entries are present and when to extend the map.apigroups/builtin_test.go (2)
9-18: Consider adding an edge case for suffix-matching boundary.The
.openshift.iosuffix check could be validated with a negative case where the substring appears but not as a suffix (e.g.,"openshift.io.example.com"). This would confirm the suffix logic works correctly.📝 Suggested additional test case
{"example.com", false}, {"widgets.example.com", false}, + {"openshift.io.example.com", false}, // contains but doesn't end with .openshift.io }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apigroups/builtin_test.go` around lines 9 - 18, Add a negative boundary test to the existing test table literal that verifies suffix-only matching: include an entry like {"openshift.io.example.com", false} (or {"something.openshift.io.example.com", false}) alongside the other cases so the suffix-check logic is exercised when ".openshift.io" appears as an internal substring but not as the domain suffix.
10-10: Use a descriptive subtest name for the empty-string case.When
tt.groupis"", the subtest name becomes empty, leading to less readable test output (Go will generate a name like#00). Consider adding anamefield to the test struct for clarity.📝 Suggested improvement for test readability
func TestIsDefaultBuiltinAPIGroup(t *testing.T) { tests := []struct { + name string group string want bool }{ - {"", true}, - {"apps", true}, - {"rbac.authorization.k8s.io", true}, - {"operators.coreos.com", true}, - {"route.openshift.io", true}, - {"config.openshift.io", true}, - {"example.com", false}, - {"widgets.example.com", false}, + {"empty (core)", "", true}, + {"apps", "apps", true}, + {"rbac.authorization.k8s.io", "rbac.authorization.k8s.io", true}, + {"operators.coreos.com", "operators.coreos.com", true}, + {"route.openshift.io", "route.openshift.io", true}, + {"config.openshift.io", "config.openshift.io", true}, + {"example.com", "example.com", false}, + {"widgets.example.com", "widgets.example.com", false}, } for _, tt := range tests { - t.Run(tt.group, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { if got := IsDefaultBuiltinAPIGroup(tt.group); got != tt.want {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apigroups/builtin_test.go` at line 10, The subtest for the empty-group case uses tt.group as the test name which is empty; add a descriptive name field to the test case struct (e.g., add "name string" alongside "group string, expected bool") and replace uses of tt.group for t.Run with tt.name so the case {"", true} becomes something like {name: "empty-group", group: "", expected: true}; update the test table (in apigroups/builtin_test.go) and change t.Run(tt.name, ...) to improve readability while leaving the test logic (functions that reference tt.group) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apigroups/builtin_test.go`:
- Around line 9-18: Add a negative boundary test to the existing test table
literal that verifies suffix-only matching: include an entry like
{"openshift.io.example.com", false} (or {"something.openshift.io.example.com",
false}) alongside the other cases so the suffix-check logic is exercised when
".openshift.io" appears as an internal substring but not as the domain suffix.
- Line 10: The subtest for the empty-group case uses tt.group as the test name
which is empty; add a descriptive name field to the test case struct (e.g., add
"name string" alongside "group string, expected bool") and replace uses of
tt.group for t.Run with tt.name so the case {"", true} becomes something like
{name: "empty-group", group: "", expected: true}; update the test table (in
apigroups/builtin_test.go) and change t.Run(tt.name, ...) to improve readability
while leaving the test logic (functions that reference tt.group) unchanged.
In `@apigroups/builtin.go`:
- Around line 5-6: The comment above the builtinK8sAPIGroups map is misleading
because it says "in-tree Kubernetes API groups" while the map
(builtinK8sAPIGroups) includes non-in-tree groups such as operators.coreos.com,
packages.operators.coreos.com, and monitoring.coreos.com; update the comment in
apigroups/builtin.go to accurately describe the map's scope (e.g., "first-party
and commonly bundled API groups not backed by user-defined CRDs" or note
inclusion of CoreOS/operator groups) so maintainers know why those entries are
present and when to extend the map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3395f007-c062-42d0-b2b8-6751f23e5dfc
📒 Files selected for processing (2)
apigroups/builtin.goapigroups/builtin_test.go
aufi
approved these changes
Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
What’s added
apigroups/builtin.goapigroups/builtin_test.goNew exported behavior
apigroups.IsDefaultBuiltinAPIGroup(group string) boolas the shared source of truth for default built-in group detection used by CRD export filtering.Summary by CodeRabbit
New Features
Tests