Avoid copy of the oc binary, to support heterogeneous environments#30992
Avoid copy of the oc binary, to support heterogeneous environments#30992jogeo wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughReplaces copying the host Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jogeo 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: 1
🧹 Nitpick comments (1)
test/extended/apiserver/kubeconfigs.go (1)
121-128: Use POSIX whitespace classes for better portability.Lines 122 and 125–128 use
\singreppatterns. While this works in the current GNU grep environment,\sis not part of the POSIX standard for BRE/ERE and may not be reliable across different grep implementations or minimal container environments. Replace with[[:space:]]for guaranteed portability:Proposed fix
- count=$(grep -Ec "^\s*${k}:\s+" "%[1]s") + count=$(grep -Ec "^[[:space:]]*${k}:[[:space:]]+" "%[1]s") [ "$count" -eq 1 ] || { echo "expected exactly one ${k} in %[1]s, got $count" >&2; exit 1; } done -server=$(grep '^\s*server:' "%[1]s" | head -1 | awk '{print $2}') -cert=$(grep '^\s*client-certificate:' "%[1]s" | head -1 | awk '{print $2}') -key=$(grep '^\s*client-key:' "%[1]s" | head -1 | awk '{print $2}') -ca=$(grep '^\s*certificate-authority:' "%[1]s" | head -1 | awk '{print $2}') +server=$(grep -E '^[[:space:]]*server:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +cert=$(grep -E '^[[:space:]]*client-certificate:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +key=$(grep -E '^[[:space:]]*client-key:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}') +ca=$(grep -E '^[[:space:]]*certificate-authority:[[:space:]]+' "%[1]s" | head -1 | awk '{print $2}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/apiserver/kubeconfigs.go` around lines 121 - 128, The grep patterns use the non-POSIX escape `\s` which can break on some grep implementations; update the patterns in the kubeconfig validation script (the lines that call grep -Ec "^\s*${k}:\s+" and the subsequent grep calls like grep '^\s*server:' , grep '^\s*client-certificate:' , grep '^\s*client-key:' and grep '^\s*certificate-authority:') to use the POSIX whitespace class `[[:space:]]` (e.g. replace occurrences of `^\s*` and `:\s+` with the equivalent `[[:space:]]` forms) so the checks are portable across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/apiserver/kubeconfigs.go`:
- Line 133: The curl invocation that currently uses the
"${server}/api?timeout=32s" URL (seen in the call that builds the curl command
string in kubeconfigs.go) leaves connection/TLS and network stalls unbounded;
modify the curl command string to add explicit time limits such as
--connect-timeout 10 and --max-time 40 (or similar values) so connection
establishment and the overall request cannot hang indefinitely, ensuring the
constructed command that includes --cert "$cert" --key "$key" --cacert "$ca" and
the "${server}/api?timeout=32s" URL is updated to include these flags.
---
Nitpick comments:
In `@test/extended/apiserver/kubeconfigs.go`:
- Around line 121-128: The grep patterns use the non-POSIX escape `\s` which can
break on some grep implementations; update the patterns in the kubeconfig
validation script (the lines that call grep -Ec "^\s*${k}:\s+" and the
subsequent grep calls like grep '^\s*server:' , grep '^\s*client-certificate:' ,
grep '^\s*client-key:' and grep '^\s*certificate-authority:') to use the POSIX
whitespace class `[[:space:]]` (e.g. replace occurrences of `^\s*` and `:\s+`
with the equivalent `[[:space:]]` forms) so the checks are portable across
environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f545b9aa-b731-4058-b4c0-3f5eeb7dde97
📒 Files selected for processing (1)
test/extended/apiserver/kubeconfigs.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-rhcos10-techpreview |
|
@jogeo: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/be82cf50-345e-11f1-93cb-693c03651f0f-0 |
|
/retest e2e-aws-ovn-fips |
|
/test e2e-aws-ovn-fips |
|
@jogeo: The following test 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. |
|
/test e2e-aws-ovn-fips |
1 participant
Copying the oc binary around fails when the environment is a different architecture or has incompatible versions of glibc.
This change instead uses curl to connect the api endpoint, after extracting the sever and cert details from the kubeconfig.