-
Notifications
You must be signed in to change notification settings - Fork 583
Add insights v1 to payload #2632
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
base: master
Are you sure you want to change the base?
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
Hello @opokornyy! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThe pull request upgrades custom resource definitions from the v1alpha2 API version to v1. Changes include removing status subresources, designating gatherConfig and gatherers as required fields, and applying minimum length and item constraints across string and array fields. Compatibility levels are updated, and new display columns (Age) are introduced. The update script is simplified by removing shell globbing syntax and expanded to include additional CRD manifest paths across multiple API groups, reflecting the API version progression and stricter validation requirements. 🚥 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
Comment |
|
[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 |
|
/test all |
|
/hold needs to be merged after - #2631 |
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
9e70d76 to
2a9f7e0
Compare
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.
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-CustomNoUpgrade.crd.yaml (1)
110-116: UpdateminLengthto 3 to match the regex minimum character requirement.The regex
^[a-z]+[_a-z]*[a-z]([/a-z][_a-z]*)?[a-z]$requires a minimum of 3 characters (e.g., "foo" matches, but "ab" does not), yetminLength: 1allows single and two-character names that the regex will reject. UpdateminLengthto3for consistency.The regex pattern is correct:
[/a-z]is a character class that includes "/" as a valid option for the separator, and the trailing[a-z]$ensures the name ends with a lowercase letter.
🤖 Fix all issues with AI agents
In @hack/update-payload-crds.sh:
- Around line 9-10: The two glob patterns in hack/update-payload-crds.sh contain
a stray double slash; update the patterns
`operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml` and
`operator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml` to
use a single slash (i.e., `...crd-manifests/*_config-operator_*.crd*yaml`) so
they match consistently with the other patterns.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml (1)
53-73: ClarifyminItems: 1constraint for optionaldataPolicyfield.The description states
dataPolicyis "optional" and "When omitted no obfuscation is applied" (lines 55, 60), butminItems: 1(line 68) requires at least one item when the array is present. This is valid semantics (optional field, but non-empty when provided), however the description could be clearer about this constraint.Consider adding wording like "When provided, it must contain at least 1 item."
📜 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 selected for processing (7)
hack/update-payload-crds.shpayload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
🔇 Additional comments (11)
payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml (2)
20-26: API upgrade to v1 with compatibility level 1 looks good.The version bump from v1alpha2 to v1 with compatibility level 1 (stable for minimum 12 months or 3 minor releases) is appropriate for a GA API.
226-231: Breaking change:gatherConfigis now required.Making
gatherConfigrequired at the spec level is a breaking change from v1alpha2. Existing CRs without this field will fail validation after upgrade.Ensure migration documentation and upgrade path are in place for existing v1alpha2 resources.
payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml (1)
1-233: Consistent with CustomNoUpgrade variant.The schema changes (v1 upgrade, required
gatherConfig,minItems/minLengthconstraints) are identical to the CustomNoUpgrade CRD, maintaining consistency across feature sets.payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml (1)
1-233: Consistent with other InsightsDataGather variants.The schema changes are identical across all three feature set variants (CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade), ensuring API consistency.
hack/update-payload-crds.sh (1)
31-32: Path updated from v1alpha2 to v1 for insights datagathers.The path change from
insights/v1alpha2toinsights/v1correctly aligns with the CRD version upgrade.payload-manifests/crds/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml (4)
29-33: Age printer column added alongside v1 upgrade.The Age column provides useful UX when listing resources with
kubectl get. The v1 version with compatibility level 1 is appropriate.
230-233:minProperties: 1on status may cause issues.The
minProperties: 1constraint requires the status object to have at least one property when present. This could cause issues if the controller sets an empty status initially before populating fields.Verify this aligns with controller behavior - if the controller always sets at least one status field (e.g.,
startTimeor a condition) on creation, this is fine.
516-518:insightsReportnow requiresdownloadedTimeanduri.Making these fields required within
insightsReportis appropriate since the report is meaningless without knowing when it was downloaded and from where.
227-228: Breaking change:gatherersis now required in spec.This is a breaking change from v1alpha2. Ensure existing DataGather resources include the
gatherersfield or have a migration path.payload-manifests/crds/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml (1)
1-627: Consistent with CustomNoUpgrade DataGather variant.The schema changes are identical to the CustomNoUpgrade CRD, maintaining consistency across feature sets for the DataGather resource.
payload-manifests/crds/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml (1)
1-627: Consistent with other DataGather variants.The schema changes are identical across all three feature set variants, ensuring API consistency for the DataGather resource.
| operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\ | ||
| operator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\ |
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.
Typo: Double slashes in glob patterns.
Lines 9 and 10 contain double slashes (//) which appear to be typos:
operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yamloperator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml
While this may still work due to path normalization, it's inconsistent with the other patterns.
🔧 Suggested fix
- operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\
- operator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\
+ operator/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\
+ operator/v1alpha1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\ | |
| operator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml\ | |
| operator/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\ | |
| operator/v1alpha1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml\ |
🤖 Prompt for AI Agents
In @hack/update-payload-crds.sh around lines 9 - 10, The two glob patterns in
hack/update-payload-crds.sh contain a stray double slash; update the patterns
`operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml` and
`operator/v1alpha1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml` to
use a single slash (i.e., `...crd-manifests/*_config-operator_*.crd*yaml`) so
they match consistently with the other patterns.
|
/testwith openshift/insights-operator/master/periodics/e2e-gcp-techpreview openshift/insights-operator#1159 ccx/insights-operator-tests#1108 |
|
@opokornyy, |
|
@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. |
|
All the v1-related cases of Insights-Operator passed in the 4.22 cluster. |
|
@BaiyangZhou can you confirm which PRs you needed to get that testing working? |
No description provided.