[CCXDEV-15259]: Promote InsightsOnDemandDataGather and InsightConfig feature gate to default #2473
Conversation
|
Hello @opokornyy! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
opokornyy
changed the title
|
/test verify-feature-promotion |
openshift-merge-robot
added
the
needs-rebase
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
3052b14 to
5d11885
Compare
openshift-merge-robot
removed
the
needs-rebase
|
[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 |
|
@opokornyy Looking at the data, we have no testing on either metal or SNO, does this feature work on those platforms? Should we be adding testing for them? |
We already have metal jobs configured (https://github.com/openshift/release/blob/b7a2a049dacad8d85b856238b0839bb7c276ca76/ci-operator/config/openshift/insights-operator/openshift-insights-operator-release-4.21__periodics.yaml#L329) |
I was checking SNO cluster and the Insights Operator is running there by default, so I am also adding test runs for that platform. |
|
@opokornyy Thanks for looking into adding those. Both metal (all variants in the promotion requirements) and SNO tests are required. |
|
I also dealt with an issue recently where our results weren't showing up in Sippy. We had to make sure our |
|
/retest |
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
5d11885 to
d72fdf4
Compare
|
/retest |
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
d72fdf4 to
96b47c4
Compare
|
/hold The Insights Operator needs to be updated first - openshift/insights-operator#1159 |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test verify-feature-promotion |
|
/retest |
|
Still not seeing metal dual stack or ipv6 showing in here, any thoughts? |
We haven’t added these yet. If they’re required, we can onboard them as well. |
Yes please |
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
0e5d556 to
2d48f9b
Compare
📝 WalkthroughWalkthroughThis pull request graduates the Insights data gather functionality from a preview feature to a stable, default-enabled capability. The changes include promoting the InsightsConfig and InsightsOnDemandDataGather feature gates from DevPreviewNoUpgrade to the Default enablement profile. The corresponding CustomResourceDefinitions are upgraded from v1alpha2 to v1, with enhanced schema validations including minItems, minLength constraints, and required field designations. Feature gate manifests are updated to move these gates from disabled to enabled lists. The build script is modified to update CRD glob patterns to encompass the evolved resource definitions. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (29)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml(0 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml(0 hunks)config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml(0 hunks)config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml(0 hunks)features.md(2 hunks)features/features.go(1 hunks)hack/update-payload-crds.sh(2 hunks)insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml(0 hunks)insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml(0 hunks)insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml(0 hunks)insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml(0 hunks)payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers.crd.yaml(0 hunks)payload-manifests/crds/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)payload-manifests/crds/0000_10_insights_01_datagathers.crd.yaml(0 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)
💤 Files with no reviewable changes (24)
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers.crd.yaml
- insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml
- insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml
- insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml
- insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml
- payload-manifests/crds/0000_10_insights_01_datagathers.crd.yaml
- config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml
- insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml
- config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml
- insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers.crd.yaml
- insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yaml
- config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
features/features.gofeatures.mdhack/update-payload-crds.shpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-Default.yaml
🧬 Code graph analysis (1)
features/features.go (1)
config/v1/types_feature.go (3)
Default(41-41)DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
🔇 Additional comments (4)
payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
278-283: Insights feature gates correctly promoted for Hypershift default
InsightsConfigandInsightsOnDemandDataGatherare added only to the enabled list (not duplicated in disabled) and stay in alphabetical order, matching the Default enablement infeatures.go. Looks good.payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
272-277: SelfManagedHA default manifest aligns with promoted Insights feature gatesThe SelfManagedHA Default feature gate manifest now enables
InsightsConfigandInsightsOnDemandDataGather(and they are not present in the disabled list), consistent withfeatures.goand the Hypershift manifest. No issues.features/features.go (1)
524-538: Insights feature gates correctly promoted to Default in registrationBoth
FeatureGateInsightsOnDemandDataGatherandFeatureGateInsightsConfigare now enabled forconfigv1.Defaultin addition to the preview feature sets. This matches the payload manifests and intended promotion, with no side‑effects on control flow.hack/update-payload-crds.sh (1)
5-30: CRD glob extensions for config/v1 and insights/v1 look consistentThe added patterns for
config/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yamlandinsights/v1/zz_generated.crd-manifests/*_insights_*.crd*yamlalign with the existing globbing approach (scoped by component substring and consumed via thefor f in ${crd_globs}loop). This should reliably pull in the intended generated CRDs without altering script behavior elsewhere.
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
2d48f9b to
271fd74
Compare
|
/test verify-feature-promotion |
openshift-merge-robot
added
the
needs-rebase
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Remove the v1alpha2 from the payload and let the script add the v1 instead for insights CRDs. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
CCXDEV-15259-insights-promotion
branch
from
271fd74 to
49edcbd
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers.crd.yaml (1)
109-115: Regex pattern has stricter length requirements than documentation suggests and contains a semantic issue.The regex
^[a-z]+[_a-z]*[a-z]([/a-z][_a-z]*)?[a-z]$requires at least 3 characters for a gatherer-only name (test results confirm "ab" is rejected). The documentation states "lowercase letters only that may include underscores (_)" without specifying a minimum length requirement, creating a mismatch.Additionally, the character class
[/a-z]after the optional function separator is semantically unclear—it means "/" OR any letter [a-z], rather than specifically requiring a slash for the separator. While the mandatory final[a-z]$anchor constrains this in practice, the pattern is ambiguous and should use a more explicit regex structure (e.g.,(\/[a-z][_a-z]*[a-z])?for the function part).Consider simplifying to:
^[a-z][a-z_]*[a-z](\/[a-z][a-z_]*[a-z])?$if 2-character minimum names are acceptable, or explicitly document that names must be 3+ characters.
🤖 Fix all issues with AI Agents
In @payload-manifests/crds/0000_10_insights_01_datagathers.crd.yaml:
- Line 232: The CRD adds minProperties: 1 on the status object which will reject
an initially empty status; either remove the minProperties: 1 constraint from
the status schema in the CRD (delete the minProperties: 1 line under the status
definition) or update the Insights controller (the code that manages the
resource status) to always set at least one status field on creation (populate a
well-known status field such as conditions or observedGeneration) so the
resource never has an empty {} status; choose one approach and apply it
consistently (edit the CRD to remove minProperties or modify the controller
status update logic).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha2/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*insights/v1alpha1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*insights/v1alpha2/zz_generated.crd-manifests/0000_10_insights_01_datagathers-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*
📒 Files selected for processing (8)
features.mdfeatures/features.gohack/update-payload-crds.shpayload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_insightsdatagathers.crd.yamlpayload-manifests/crds/0000_10_insights_01_datagathers.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- features/features.go
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- features.md
🔇 Additional comments (8)
payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
266-271: LGTM!The InsightsConfig and InsightsOnDemandDataGather feature gates are correctly added to the enabled list in alphabetical order, consistent with the PR objective of promoting these features to Default.
payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers.crd.yaml (2)
19-25: LGTM!The API version promotion from v1alpha2 to v1 with updated compatibility level description correctly reflects the feature graduation to GA status.
67-67: VerifyminItems: 1does not break optional field semantics.The
dataPolicyfield is described as optional ("When omitted no obfuscation is applied"), but addingminItems: 1means if the field is present, it must have at least one item. This is correct semantics—just ensure consumers don't create empty arrays.payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-Default.crd.yaml (1)
1-233: LGTM!The new Default-annotated CRD correctly introduces the
release.openshift.io/feature-set: Defaultannotation and maintains schema consistency with the base CRD. This aligns with the feature promotion requirements.payload-manifests/crds/0000_10_insights_01_datagathers.crd.yaml (2)
28-32: LGTM!The addition of the Age printer column and version update to v1 are appropriate for the GA promotion.
145-153: DataGather CRD missing "None" mode option from config-operator CRD.The
modeenum in this CRD contains only[All, Custom], while the InsightsDataGather CRD (in0000_10_config-operator_01_insightsdatagathers.crd.yaml) includes[All, None, Custom]. The config-operator CRD uses "None" to disable all gatherers. Confirm whether the DataGather CRD intentionally omits this option or if it should be added for consistency.hack/update-payload-crds.sh (2)
7-7: LGTM!The broader glob pattern correctly accommodates the new InsightsDataGather CRD files in the config/v1 directory.
31-31: The glob pattern correctly matches the insights CRD naming convention.The new pattern
insights/v1/zz_generated.crd-manifests/*_insights_*.crd*yamlwill match the expected insights v1 CRD file0000_10_insights_01_datagathers.crd.yaml. The glob is appropriately scoped to the insights API group and follows the same structure as other CRD patterns in the script. Note that the insightsdatagathers CRDs from config/v1 (withconfig-operator_01_insightsdatagathersnaming) are already being copied via the existing patternconfig/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml.
|
@opokornyy: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
No description provided.