CNF-23378: Migrate away from deprecated ioutil - #156
Conversation
d35f3ed to
04d9547
Compare
|
/retest |
04d9547 to
8264f7d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces deprecated ChangesGo standard library API migration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/cmd/oauth-apiserver/testing/integration.go (2)
14-60:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix nil-pointer panic:
storageConfigcan be nil inStartDefaultTestServer.
StartDefaultIntegrationTestServerpassesstorageConfig=nilintoStartDefaultTestServer, butStartDefaultTestServerunconditionally accessesstorageConfig.EventsHistoryWindow(Line 40). This will panic at runtime.🛠️ Proposed fix (make the EventsHistoryWindow override nil-safe)
- fakeKubeConfig.Close() - storageConfig.EventsHistoryWindow = max(storageConfig.EventsHistoryWindow, storagebackend.DefaultEventsHistoryWindow) + fakeKubeConfig.Close() + if storageConfig != nil { + storageConfig.EventsHistoryWindow = max(storageConfig.EventsHistoryWindow, storagebackend.DefaultEventsHistoryWindow) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/oauth-apiserver/testing/integration.go` around lines 14 - 60, StartDefaultTestServer dereferences storageConfig.EventsHistoryWindow which can be nil; update StartDefaultTestServer to handle a nil storageConfig by creating or using a local default storagebackend.Config when storageConfig == nil (or by guarding the access) before calling max(storageConfig.EventsHistoryWindow, storagebackend.DefaultEventsHistoryWindow) so the code never dereferences a nil pointer; adjust any later use of storageConfig (passed into StartTestServer) to pass the defaulted/local config instead of nil.
20-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle temp kubeconfig
WriteString/Closeerrors instead of ignoring them.Currently the return values from
fakeKubeConfig.WriteString(...)(Lines 20-38) andfakeKubeConfig.Close()(Line 39) are not checked. If those operations fail, the subsequent server startup can fail in confusing ways.🛠️ Proposed fix (check errors and clean up the temp file)
- fakeKubeConfig.WriteString(` + if _, err := fakeKubeConfig.WriteString(` apiVersion: v1 kind: Config clusters: - cluster: server: http://127.1.2.3:12345 name: integration contexts: - context: cluster: integration user: test name: default-context current-context: default-context users: - name: test user: password: test username: test `) - fakeKubeConfig.Close() + ); err != nil { + name := fakeKubeConfig.Name() + _ = fakeKubeConfig.Close() + _ = os.Remove(name) + return nil, nil, err + } + + if err := fakeKubeConfig.Close(); err != nil { + name := fakeKubeConfig.Name() + _ = os.Remove(name) + return nil, nil, err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/oauth-apiserver/testing/integration.go` around lines 20 - 39, The temporary kubeconfig file operations currently ignore errors from fakeKubeConfig.WriteString(...) and fakeKubeConfig.Close(), which can mask failures; update the setup to check the returned (n, err) from fakeKubeConfig.WriteString and handle any error (fail the test or return the error), and likewise check the error returned by fakeKubeConfig.Close(); on any error ensure the temp file is cleaned up (remove it) and propagate/fail appropriately so server startup won't proceed with a bad kubeconfig; reference the fakeKubeConfig variable and its WriteString and Close calls when making these changes.test/e2e/tokenreviews.go (1)
51-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
resp.StatusCodeassertions to make failures more actionable.After each
insecureClient.Do(...), add arequire.Equal(t, http.StatusOK, resp.StatusCode)before reading/unmarshalling the body. Otherwise, if the endpoint returns a non-200 (authn/authz/proxy issues), the test may fail later with JSON/unmarshal noise instead of an obvious HTTP status mismatch.🛠️ Proposed fix
resp, err := insecureClient.Do(failedReviewReq) require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) defer resp.Body.Close() respBodyBytes, err := io.ReadAll(resp.Body) require.NoError(t, err)resp, err = insecureClient.Do(successfulReviewReq) require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) defer resp.Body.Close() respBodyBytes, err = io.ReadAll(resp.Body) require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/tokenreviews.go` around lines 51 - 80, After each call to insecureClient.Do (used with createTokenReviewRequestForToken for both "notaveryrandomnameforanything" and accessToken.Name) assert the HTTP status code is OK before reading the body: check resp.StatusCode equals http.StatusOK and fail the test early if not, so subsequent io.ReadAll and json.Unmarshal on tokenReviewResp are only executed on successful 200 responses; add this require.Equal(t, http.StatusOK, resp.StatusCode) immediately after each insecureClient.Do and before resp.Body.Close()/io.ReadAll and unmarshalling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/cmd/oauth-apiserver/testing/integration.go`:
- Around line 14-60: StartDefaultTestServer dereferences
storageConfig.EventsHistoryWindow which can be nil; update
StartDefaultTestServer to handle a nil storageConfig by creating or using a
local default storagebackend.Config when storageConfig == nil (or by guarding
the access) before calling max(storageConfig.EventsHistoryWindow,
storagebackend.DefaultEventsHistoryWindow) so the code never dereferences a nil
pointer; adjust any later use of storageConfig (passed into StartTestServer) to
pass the defaulted/local config instead of nil.
- Around line 20-39: The temporary kubeconfig file operations currently ignore
errors from fakeKubeConfig.WriteString(...) and fakeKubeConfig.Close(), which
can mask failures; update the setup to check the returned (n, err) from
fakeKubeConfig.WriteString and handle any error (fail the test or return the
error), and likewise check the error returned by fakeKubeConfig.Close(); on any
error ensure the temp file is cleaned up (remove it) and propagate/fail
appropriately so server startup won't proceed with a bad kubeconfig; reference
the fakeKubeConfig variable and its WriteString and Close calls when making
these changes.
In `@test/e2e/tokenreviews.go`:
- Around line 51-80: After each call to insecureClient.Do (used with
createTokenReviewRequestForToken for both "notaveryrandomnameforanything" and
accessToken.Name) assert the HTTP status code is OK before reading the body:
check resp.StatusCode equals http.StatusOK and fail the test early if not, so
subsequent io.ReadAll and json.Unmarshal on tokenReviewResp are only executed on
successful 200 responses; add this require.Equal(t, http.StatusOK,
resp.StatusCode) immediately after each insecureClient.Do and before
resp.Body.Close()/io.ReadAll and unmarshalling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cec8a7f8-f929-4f6d-b92d-8a156dfe0928
📒 Files selected for processing (3)
pkg/cmd/oauth-apiserver/testing/integration.gopkg/cmd/oauth-apiserver/testing/testserver.gotest/e2e/tokenreviews.go
sebrandon1
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CNF-23378 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. |
|
/retest |
8264f7d to
b5d4ffc
Compare
|
[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 |
|
/retest |
b5d4ffc to
7c515f8
Compare
7c515f8 to
8adde91
Compare
8adde91 to
66b8c3f
Compare
|
@sebrandon1: 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. |
66b8c3f to
2a7e2cf
Compare
|
Progress update: 5 of 19 PRs in this ioutil migration effort have merged! 🎉 Recently merged:
This PR has been rebased against the latest upstream and is ready for review. Epic: CNF-23315 |
2 participants
ioutilhas been deprecated since Go 1.16: https://go.dev/doc/go1.16#ioutilTracking issue: redhat-best-practices-for-k8s/telco-bot#52
Migration from
ioutiltoos/io:ioutil.TempFilewithos.CreateTempinintegration.gofor creating temporary kubeconfig files.ioutil.TempDirwithos.MkdirTempintestserver.gofor creating temporary directories.integration.go,testserver.go, andtokenreviews.goto removeioutiland addosorioas needed. [1] [2] [3]ioutil.ReadAllwithio.ReadAllintokenreviews.gofor reading HTTP response bodies. [1] [2]Summary by CodeRabbit