[Debug]Rebase the v1 PR to build the cluster#2692
[Debug]Rebase the v1 PR to build the cluster#2692BaiyangZhou wants to merge 4 commits intoopenshift:masterfrom
Conversation
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @BaiyangZhou! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughCRD field annotations were changed: 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Review Summary by QodoRemove insights feature gates and fix v1alpha1 validation errors
WalkthroughsDescription• Remove feature gates for InsightsConfig and InsightsOnDemandDataGather • Add Kubernetes list type annotations to array fields • Make namespace field required in ObjectReference • Enable insights features in Default and OKD configurations Diagramflowchart LR
FG["Feature Gates<br/>InsightsConfig<br/>InsightsOnDemandDataGather"]
FG -- "Remove from<br/>TechPreview" --> Default["Enable in Default<br/>and OKD"]
Types["Type Definitions<br/>insights/v1alpha1"]
Types -- "Add list type<br/>annotations" --> CRD["CRD Manifests<br/>with x-kubernetes<br/>extensions"]
ObjRef["ObjectReference<br/>namespace field"]
ObjRef -- "Change to required" --> Validation["Fix v1alpha1<br/>validation errors"]
File Changes1. features/features.go
|
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
insights/v1alpha1/types_insights.go (1)
332-337:⚠️ Potential issue | 🟠 MajorRemove
omitemptyfrom the requiredNamespacefield to prevent silent omission.The
Namespacefield is marked+requiredbut still hasomitemptyin the JSON tag, creating a contradiction. When marshaled to JSON, empty values will be dropped despite the requirement marker. This is also inconsistent with the other required fields in the same struct (Group,Resource,Name), which are all tagged withoutomitempty.🔧 Suggested fix
- Namespace string `json:"namespace,omitempty"` + Namespace string `json:"namespace"`
Code Review by Qodo
1. Undocumented +listType markers
|
Review Summary by QodoRemove Insights feature gates and fix v1alpha1 schema validation
WalkthroughsDescription• Remove feature gates for InsightsConfig and InsightsOnDemandDataGather • Add Kubernetes list type annotations for proper schema validation • Make namespace field required in ObjectReference type • Enable Insights features in Default and OKD configurations Diagramflowchart LR
A["Feature Gate Removal"] --> B["InsightsConfig & InsightsOnDemandDataGather"]
C["Schema Validation"] --> D["List Type Annotations"]
C --> E["Required Namespace Field"]
F["Feature Enablement"] --> G["Default & OKD Profiles"]
B --> H["Updated CRD Manifests"]
D --> H
E --> H
G --> H
File Changes1. features/features.go
|
|
Persistent review updated to latest commit 8150933 |
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/features.go (1)
500-514:⚠️ Potential issue | 🟡 MinorPromoting
InsightsOnDemandDataGatherandInsightsConfigtoDefault/OKDis a significant graduation — confirm the operators are ready.Both feature gates are now enabled in
DefaultandOKD(previously only inDevPreviewNoUpgrade/TechPreviewNoUpgrade). This makes the features active on production clusters by default.Concerns:
- These are marked as legacy feature gates in
legacyfeaturegates.go(with a "never add to this list" comment), yet they're being promoted. Clarify whether this exception is justified.- Confirm the insights-operator and config-operator are ready for GA-level enablement and that all related controllers can handle these gates being universally enabled.
Note: CRD manifests for these gates remain in the repository (not removed), so the infrastructure is in place.
3 participants
No description provided.