NE-2826: Fix staticcheck warnings across multiple packages - #815
NE-2826: Fix staticcheck warnings across multiple packages#815ankimaha-sys wants to merge 1 commit into
Conversation
|
@ankimaha-sys: This pull request explicitly references no jira issue. DetailsIn 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @ankimaha-sys. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
WalkthroughThe changes align Kubernetes type references, make shutdown and CRL waits directly block on channels, and simplify endpoint slice allocation and unauthorized response construction. ChangesRouter updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
14b712a to
e0a914e
Compare
|
/approve |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Address several staticcheck findings: - Remove unnecessary fmt.Sprintf wrapping a plain string (S1039) - Simplify single-case select to direct channel receive (S1000) - Remove redundant capacity argument in make (S1019) - Consolidate duplicate package imports (ST1019) All changes are mechanical and do not alter runtime behavior. Signed-off-by: Ankit Mahajan <ankimaha@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
e0a914e to
815e9a6
Compare
|
/retest |
|
/retest-required |
|
/retest |
|
/retest-required |
|
@knobunc @Thealisyed Could one of you please /lgtm and /retest the last flaky e2e-agnostic job (1 flaky failure out of 2204 tests)? All code checks are green. Thanks! |
| select { | ||
| case <-stopCh: | ||
| cancel() | ||
| // 45s is the default interval that almost all cloud load balancers require to take an unhealthy | ||
| // endpoint out of rotation. | ||
| delay := getIntervalFromEnv("ROUTER_GRACEFUL_SHUTDOWN_DELAY", 45) | ||
| log.Info(fmt.Sprintf("Shutdown requested, waiting %s for new connections to cease", delay)) | ||
| time.Sleep(delay) | ||
| log.Info("Instructing the template router to terminate") | ||
| if err := templatePlugin.Stop(); err != nil { | ||
| log.Error(err, "Router did not shut down cleanly") | ||
| } else { | ||
| log.Info("Shutdown complete, exiting") | ||
| } | ||
| // wait one second to let any remaining actions settle | ||
| time.Sleep(time.Second) | ||
| <-stopCh | ||
| cancel() | ||
| // 45s is the default interval that almost all cloud load balancers require to take an unhealthy | ||
| // endpoint out of rotation. | ||
| delay := getIntervalFromEnv("ROUTER_GRACEFUL_SHUTDOWN_DELAY", 45) | ||
| log.Info(fmt.Sprintf("Shutdown requested, waiting %s for new connections to cease", delay)) | ||
| time.Sleep(delay) | ||
| log.Info("Instructing the template router to terminate") | ||
| if err := templatePlugin.Stop(); err != nil { | ||
| log.Error(err, "Router did not shut down cleanly") | ||
| } else { | ||
| log.Info("Shutdown complete, exiting") | ||
| } | ||
| // wait one second to let any remaining actions settle | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
I like having the select statement as a way of logically grouping the wait and shutdown logic. That said, this change is functionally correct, and if the logic should be grouped, perhaps a better way to do so would be to add a comment and blank line or to refactor the shutdown logic into its own function.
@coderabbitai, I believe you are using confusing language, and that these changes result in no change in behavior (other than avoiding an unnecessary alloc), but I want to make sure I am not missing something. I expect that deleting
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR diff ---'
git diff --unified=12 HEAD^ HEAD -- \
pkg/cmd/infra/router/clientcmd.go \
pkg/cmd/infra/router/template.go \
pkg/router/controller/factory/factory.go \
pkg/router/controller/status.go \
pkg/router/crl/crl.go \
pkg/router/metrics/metrics.go
echo '--- relevant declarations/call contexts ---'
ast-grep outline pkg/cmd/infra/router/template.go --items all --match 'Run' --view expanded || true
ast-grep outline pkg/router/crl/crl.go --items all --match 'ManageCRLs' --view expanded || true
ast-grep outline pkg/router/metrics/metrics.go --items all --match 'authorizeHandler' --view expanded || true
ast-grep outline pkg/router/controller/factory/factory.go --items all --match 'aggregateEndpointSlice' --view expanded || trueLength of output: 10834
Specifically:
Thank you for calling this out. A more accurate summary would say: “Simplify staticcheck-flagged constructs and consolidate duplicate imports without changing runtime behavior.” ✏️ Learnings added
|
openshift-ci
Bot
changed the title
|
@ankimaha-sys: This pull request references NE-2826 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn 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. |
|
@coderabbitai, please check whether https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_router/815/pull-ci-openshift-router-master-e2e-agnostic/2079964201975025664 failed because of a known issue. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
url='https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_router/815/pull-ci-openshift-router-master-e2e-agnostic/2079964201975025664'
echo '--- Prow job page (HTTP status and relevant links) ---'
curl -fsSL "$url" -o /tmp/prow-job.html
grep -Eio 'https?://[^" ]+|href="[^"]+"' /tmp/prow-job.html \
| grep -Ei 'build-log|artifacts|junit|finished|failure|gcs|test-platform-results' \
| head -100 || true
echo '--- Page text around failure-related terms ---'
grep -Ein -C 3 'fail|error|junit|artifact|build-log' /tmp/prow-job.html | head -240 || true
echo '--- Common directly-addressable artifacts ---'
base='https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_router/815/pull-ci-openshift-router-master-e2e-agnostic/2079964201975025664'
for artifact in build-log.txt finished.json started.json; do
echo "### ${artifact}"
curl -fsSL "${base}/${artifact}" | tail -n 300 || true
doneLength of output: 35062 🏁 Script executed: #!/bin/bash
set -euo pipefail
prefix='https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_router/815/pull-ci-openshift-router-master-e2e-agnostic/2079964201975025664'
junit_path='artifacts/e2e-agnostic/openshift-e2e-test/artifacts/junit/junit_e2e__20260722-172519.xml'
curl -fsSL "$prefix/build-log.txt" -o /tmp/build-log.txt
curl -fsSL "$prefix/$junit_path" -o /tmp/junit.xml
echo '--- Exact build-log context ---'
grep -in -A 45 -B 15 'TestDeployScale' /tmp/build-log.txt || true
echo '--- Failing JUnit testcase (name, duration, failure output) ---'
python3 - <<'PY'
import xml.etree.ElementTree as ET
root = ET.parse('/tmp/junit.xml').getroot()
for case in root.iter('testcase'):
if 'TestDeployScale' in case.attrib.get('name', ''):
print('name:', case.attrib.get('name'))
print('classname:', case.attrib.get('classname'))
print('time:', case.attrib.get('time'))
for child in case:
if child.tag in ('failure', 'error', 'system-out', 'system-err'):
print(f'--- {child.tag} ---')
print((child.text or '').strip())
PY
echo '--- Prow job history page: recent status rows ---'
curl -fsSL 'https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-router-master-e2e-agnostic' \
-o /tmp/job-history.html
grep -Ein -C 2 'TestDeployScale|failure|failed|success|passed' /tmp/job-history.html | head -200 || true
echo '--- Public GitHub issues/PRs mentioning the exact failing test ---'
gh api -H 'Accept: application/vnd.github+json' \
'/search/issues?q=repo%3Aopenshift%2Forigin+%22TestDeployScale%22+is%3Aissue' \
--jq '.items[] | [.state, .number, .title, .html_url] | `@tsv`' 2>/dev/null || true
gh api -H 'Accept: application/vnd.github+json' \
'/search/issues?q=repo%3Aopenshift%2Forigin+%22TestDeployScale%22+is%3Apr' \
--jq '.items[] | [.state, .number, .title, .html_url] | `@tsv`' 2>/dev/null || trueLength of output: 52574 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- Prior TestDeployScale flake reports ---'
for issue in 9443 13439; do
echo "### openshift/origin#${issue}"
gh api "repos/openshift/origin/issues/${issue}" \
--jq '"title: \(.title)\nstate: \(.state)\ncreated: \(.created_at)\nclosed: \(.closed_at)\nurl: \(.html_url)\nbody:\n\(.body)"' \
2>/dev/null || true
done
echo '--- Public issue/PR search for this exact resource-version conflict ---'
for query in \
'repo:openshift/origin "TestDeployScale" "object has been modified"' \
'repo:openshift/origin "deploymentconfigs.apps.openshift.io" "object has been modified"' \
'"TestDeployScale" "Operation cannot be fulfilled on deploymentconfigs.apps.openshift.io"'; do
echo "### ${query}"
gh api -H 'Accept: application/vnd.github+json' "/search/issues?q=$(printf '%s' "$query" | jq -sRr `@uri`)" \
--jq '.items[] | [.state, .number, .title, .html_url] | `@tsv`' 2>/dev/null || true
done
echo '--- Same signature in recent router e2e-agnostic jobs ---'
curl -fsSL 'https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-router-master-e2e-agnostic' \
-o /tmp/router-job-history.html
python3 - <<'PY'
import json, re, html, urllib.request
text = open('/tmp/router-job-history.html', encoding='utf-8').read()
m = re.search(r'var allBuilds = (\[.*?\]);', text, re.S)
builds = json.loads(html.unescape(m.group(1)))
for b in builds[:25]:
if b['Result'] != 'FAILURE':
continue
link = 'https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/' + b['SpyglassLink'].split('/view/gs/',1)[1] + '/build-log.txt'
try:
log = urllib.request.urlopen(link, timeout=20).read().decode('utf-8', 'replace')
except Exception as e:
print(f"{b['ID']}\t{b['Result']}\tartifact unavailable: {e}")
continue
if 'Operation cannot be fulfilled on deploymentconfigs.apps.openshift.io "config": the object has been modified' in log:
pr = b['Refs']['pulls'][0]['number']
print(f"{b['ID']}\tPR #{pr}\tSAME TestDeployScale conflict\t{b['SpyglassLink']}")
PYLength of output: 9682
The only blocking failure was:
That is the same DeploymentConfig update-conflict signature documented in openshift/origin#9443, “TestDeployScale test-integration flake.” The failure occurs in the upstream OpenShift e2e test at A caveat: Given that this PR contains mechanical staticcheck cleanups and another |
|
Let's rerun the test and file a report if the same failure happens again. /test e2e-agnostic |
|
@ankimaha-sys: all tests passed! 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. |
4 participants
Description
Address several staticcheck findings across the router codebase:
fmt.Sprintfwrapping a plain string (metrics.go)selectto direct channel receive (template.go,crl.go)make(factory.go)clientcmd.go,status.go)Changes
pkg/router/metrics/metrics.go: Replacefmt.Sprintf("Unauthorized")with string literalpkg/cmd/infra/router/template.go: Replace single-caseselectwith direct<-stopChreceivepkg/router/crl/crl.go: Replace single-caseselectwith direct channel receivepkg/router/controller/factory/factory.go: Remove redundant cap inmake([]T, len, len)pkg/cmd/infra/router/clientcmd.go: Remove duplicate import ofk8s.io/client-go/tools/clientcmdpkg/router/controller/status.go: Remove duplicate import ofk8s.io/api/core/v1All changes are mechanical and do not alter runtime behavior.
Summary by CodeRabbit