Revert "Merge pull request #2448 from opokornyy/CCXDEV-15259-io-v1" #2608
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @opokornyy! Some important instructions when contributing to openshift/api: |
WalkthroughThis PR removes two API versions: InsightsDataGather from config/v1 and DataGather from insights/v1. The changes encompass deletion of type definitions, registration logic, CRD manifests, generated code (deepcopy, Swagger documentation, OpenAPI definitions), and associated test files. The scheme builder is updated to exclude the removed API version. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)openapi/generated_openapi/zz_generated.openapi.go (1)
🔇 Additional comments (2)
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 |
openshift-ci
bot
added
the
size/XXL
![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 (32)
config/v1/register.go(0 hunks)config/v1/tests/insightsdatagathers.config.openshift.io/InsightsConfig.yaml(0 hunks)config/v1/types_insights.go(0 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml(0 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)config/v1/zz_generated.deepcopy.go(0 hunks)config/v1/zz_generated.featuregated-crd-manifests.yaml(0 hunks)config/v1/zz_generated.featuregated-crd-manifests/insightsdatagathers.config.openshift.io/InsightsConfig.yaml(0 hunks)config/v1/zz_generated.swagger_doc_generated.go(0 hunks)hack/update-payload-crds.sh(1 hunks)insights/install.go(1 hunks)insights/v1/Makefile(0 hunks)insights/v1/doc.go(0 hunks)insights/v1/register.go(0 hunks)insights/v1/tests/datagathers.insights.openshift.io/InsightsOnDemandDataGather.yaml(0 hunks)insights/v1/types_insights.go(0 hunks)insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml(0 hunks)insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml(0 hunks)insights/v1/zz_generated.crd-manifests/doc.go(0 hunks)insights/v1/zz_generated.deepcopy.go(0 hunks)insights/v1/zz_generated.featuregated-crd-manifests.yaml(0 hunks)insights/v1/zz_generated.featuregated-crd-manifests/datagathers.insights.openshift.io/InsightsOnDemandDataGather.yaml(0 hunks)insights/v1/zz_generated.swagger_doc_generated.go(0 hunks)openapi/openapi.json(0 hunks)payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.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_insights_01_datagathers-CustomNoUpgrade.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)
💤 Files with no reviewable changes (30)
- config/v1/register.go
- insights/v1/tests/datagathers.insights.openshift.io/InsightsOnDemandDataGather.yaml
- insights/v1/zz_generated.crd-manifests/doc.go
- insights/v1/Makefile
- insights/v1/register.go
- payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml
- config/v1/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_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yaml
- insights/v1/doc.go
- payload-manifests/crds/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1/tests/insightsdatagathers.config.openshift.io/InsightsConfig.yaml
- config/v1/types_insights.go
- payload-manifests/crds/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml
- insights/v1/zz_generated.featuregated-crd-manifests/datagathers.insights.openshift.io/InsightsOnDemandDataGather.yaml
- insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-CustomNoUpgrade.crd.yaml
- insights/v1/zz_generated.crd-manifests/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_insights_01_datagathers-DevPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.featuregated-crd-manifests/insightsdatagathers.config.openshift.io/InsightsConfig.yaml
- insights/v1/zz_generated.featuregated-crd-manifests.yaml
- insights/v1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_10_insights_01_datagathers-TechPreviewNoUpgrade.crd.yaml
- config/v1/zz_generated.deepcopy.go
- config/v1/zz_generated.swagger_doc_generated.go
- openapi/openapi.json
- insights/v1/zz_generated.deepcopy.go
- insights/v1/types_insights.go
🧰 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:
hack/update-payload-crds.shinsights/install.go
🧬 Code graph analysis (1)
insights/install.go (3)
config/v1/register.go (1)
Install(14-14)insights/v1alpha2/register.go (1)
Install(14-14)insights/v1alpha1/register.go (1)
Install(14-14)
🔇 Additional comments (1)
insights/install.go (1)
16-16: Verify removal of all insights/v1 dependencies.The removal of
insightsv1.Installfrom the scheme builder is correct and aligns with the broader v1 API removal in this revert. However, ensure no code in the repository still depends on insights/v1 resources being registered in the runtime scheme. Check for any remaining references to the insights/v1 API package and insightsv1 types across the codebase before merging.
| 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.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and read its content
cat -n hack/update-payload-crds.shRepository: openshift/api
Length of output: 170
🏁 Script executed:
# Check if the directories mentioned in the patterns exist
find . -type d -name "zz_generated.crd-manifests" 2>/dev/null | head -10Repository: openshift/api
Length of output: 170
🏁 Script executed:
# Test glob expansion with double slashes vs single slashes in bash
# (simulating the patterns without requiring the actual directories)
python3 << 'EOF'
import os
import glob
# Test how bash glob behaves with double slashes
# In Python's glob, // is treated as / (POSIX standard)
patterns = [
"operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml",
"operator/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml"
]
print("Testing Python glob behavior:")
for pattern in patterns:
result = glob.glob(pattern)
print(f"Pattern: {pattern}")
print(f"Matches: {len(result)} files")
if result:
print(f"Examples: {result[:2]}")
print()
# Test bash behavior
import subprocess
bash_code = '''
# Test bash globbing with double slash
shopt -s nullglob # Don't error if no match
echo "=== Double slash pattern ==="
for file in operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml; do
echo "$file"
done | head -3
echo "Count: $(echo operator/v1/zz_generated.crd-manifests//*_config-operator_*.crd*yaml | wc -w)"
echo ""
echo "=== Single slash pattern ==="
for file in operator/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml; do
echo "$file"
done | head -3
echo "Count: $(echo operator/v1/zz_generated.crd-manifests/*_config-operator_*.crd*yaml | wc -w)"
'''
print("Testing bash glob behavior:")
result = subprocess.run(['bash', '-c', bash_code], capture_output=True, text=True, cwd='.')
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
EOFRepository: openshift/api
Length of output: 170
🌐 Web query:
bash glob expansion double slashes pathname normalization POSIX
💡 Result:
Short answer
- POSIX: multiple successive slashes in a pathname are treated as a single slash, except a pathname beginning with exactly two leading slashes (“//...”) may be interpreted in an implementation‑defined way. [1][2]
- Globbing: when matching pathnames the slash (/) must be matched explicitly (globs do not implicitly match across slashes). Bash (and some implementations) may collapse redundant slashes when performing pathname expansion, but that trimming is not required by POSIX and other shells/implementations may not do it. Relying on collapse/normalization is therefore non‑portable. [3][4][5]
Relevant details and implications
- POSIX text: “Multiple successive characters are considered to be the same as one , except for the case of exactly two leading characters” and “a pathname that begins with two successive slashes may be interpreted in an implementation‑defined manner.” Use caution with leading “//”. [1][2]
- Shell globbing: the slash must be matched explicitly in pathname expansion; shells expand globs against filenames, not by silently crossing directory separators. [3]
- Implementation differences: bash historically normalizes some redundant slashes in glob results (and its PWD handling may preserve “//”), but other shells (dash, ksh, zsh, etc.) behave differently. Don’t depend on automatic normalization if portability matters. [4][5]
If you want deterministic behavior
- Normalize paths explicitly in your script (e.g., resolve with realpath/readlink -f where available, or use a small normalization routine) rather than relying on shell globbing to collapse slashes.
- Avoid generating pathnames that begin with “//” unless you intend implementation‑specific semantics.
Sources
[1] POSIX Pathname resolution and rationale (The Open Group). (pubs.opengroup.org)
[2] POSIX discussion / std quote on multiple slashes (Unix & Linux Q/A). (unix.stackexchange.com)
[3] Bash pathname expansion / slash matching (Bash Hackers’ Wiki). (flokoe.github.io)
[4] Evidence and discussion of bash collapsing redundant slashes / portability notes. (unix.stackexchange.com)
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/007904975/xrat/xbd_chap04.html?utm_source=openai
- 2: https://unix.stackexchange.com/questions/320435/is-it-ok-that-find-displays-double-forward-slash?utm_source=openai
- 3: https://flokoe.github.io/bash-hackers-wiki/syntax/expansion/globs/?utm_source=openai
- 4: https://unix.stackexchange.com/questions/775154/behavior-of-truncating-multiple-slashes-in-paths-stemming-from-glob-pattern-matc?utm_source=openai
Inconsistent slash usage should be standardized for clarity and portability.
Lines 9–10 use double slashes in glob patterns (zz_generated.crd-manifests//*_config-operator_), while other lines (6–8, 11–13) use single slashes. While bash typically normalizes consecutive slashes, this behavior is not guaranteed across all shells and reduces clarity.
For consistency and portability, use single slashes throughout:
- 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 to 10, the glob patterns use
double slashes ("zz_generated.crd-manifests//*_config-operator_*.crd*yaml")
which is inconsistent with the single-slash patterns used elsewhere; replace the
double slashes with single slashes so both lines use the same
"zz_generated.crd-manifests/*_config-operator_*.crd*yaml" form to ensure
consistency and portability across shells.
| 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 |
There was a problem hiding this comment.
Do we need to keep these for now since the inclusion from the insights operator repo got removed?
There was a problem hiding this comment.
Yes, I have added this back.
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
|
@opokornyy: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
|
/lgtm Reverting so that this isn't in master when we branch, will unrevert once branched /hold for WIP client-go PR to go in shortly after this |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/verified by openshift/client-go#352 |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-ci-robot
added
the
verified
|
@JoelSpeed: This PR has been marked as verified by In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-client-go In response to this:
Instructions 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. |
3 participants
This reverts commit 595e66a, reversing changes made to 172bfaf.