CNF-23374: Migrate away from deprecated ioutil#202
Open
sebrandon1 wants to merge 1 commit into
Open
Conversation
Contributor
|
/approve |
openshift-ci
Bot
added
the
approved
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ibihim, liouk, sebrandon1 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-merge-robot
added
the
needs-rebase
sebrandon1
force-pushed
the
ioutil_deprecation
branch
from
3229315 to
a309d38
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
openshift-merge-robot
removed
the
needs-rebase
sebrandon1
force-pushed
the
ioutil_deprecation
branch
from
a309d38 to
44f7d10
Compare
Member
Author
|
/retest |
sebrandon1
force-pushed
the
ioutil_deprecation
branch
from
44f7d10 to
472e50b
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactor replacing deprecated ChangesReplace ioutil usage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/oauthserver/oauth_apiserver_test.go (1)
29-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose
os.CreateTempfile handles beforeos.WriteFile(prevents leaks/flaky cleanup).In each test,
os.CreateTempreturns an open*os.File, but it’s never closed before writing viaos.WriteFile(tmpfile.Name(), ...). This is a resource leak and can cause flaky failures on platforms that restrict deleting/opening temp files (commonly Windows), sinceos.Removeis deferred.🔧 Proposed fix
func TestGetInvalidSessionSecretsFile(t *testing.T) { - tmpfile, err := os.CreateTemp("", "invalid.yaml") + tmpfile, err := os.CreateTemp("", "invalid.yaml") if err != nil { t.Fatalf("unexpected error: %v", err) } - defer os.Remove(tmpfile.Name()) + name := tmpfile.Name() + if err := tmpfile.Close(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.Remove(name) - if err := os.WriteFile(tmpfile.Name(), []byte("invalid content"), os.FileMode(0600)); err != nil { + if err := os.WriteFile(name, []byte("invalid content"), os.FileMode(0600)); err != nil { t.Fatal(err) } - _, err = getSessionSecrets(tmpfile.Name()) + _, err = getSessionSecrets(name) if err == nil { t.Errorf("Expected error, got none") } } func TestGetEmptySessionSecretsFile(t *testing.T) { - tmpfile, err := os.CreateTemp("", "empty.yaml") + tmpfile, err := os.CreateTemp("", "empty.yaml") if err != nil { t.Fatalf("unexpected error: %v", err) } - defer os.Remove(tmpfile.Name()) + name := tmpfile.Name() + if err := tmpfile.Close(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.Remove(name) secrets := &osinv1.SessionSecrets{ Secrets: []osinv1.SessionSecret{}, } ... - if err := os.WriteFile(tmpfile.Name(), []byte(yaml), os.FileMode(0600)); err != nil { + if err := os.WriteFile(name, []byte(yaml), os.FileMode(0600)); err != nil { t.Fatal(err) } - _, err = getSessionSecrets(tmpfile.Name()) + _, err = getSessionSecrets(name) if err == nil { t.Errorf("Expected error, got none") } } func TestGetValidSessionSecretsFile(t *testing.T) { - tmpfile, err := os.CreateTemp("", "valid.yaml") + tmpfile, err := os.CreateTemp("", "valid.yaml") if err != nil { t.Fatalf("unexpected error: %v", err) } - defer os.Remove(tmpfile.Name()) + name := tmpfile.Name() + if err := tmpfile.Close(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.Remove(name) ... - if err := os.WriteFile(tmpfile.Name(), []byte(yaml), os.FileMode(0600)); err != nil { + if err := os.WriteFile(name, []byte(yaml), os.FileMode(0600)); err != nil { t.Fatal(err) } - readSecrets, err := getSessionSecrets(tmpfile.Name()) + readSecrets, err := getSessionSecrets(name) if err != nil { t.Errorf("Unexpected error: %v", err) } if !reflect.DeepEqual(readSecrets, expectedSecrets) { t.Errorf("Unexpected %v, got %v", expectedSecrets, readSecrets) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/oauthserver/oauth_apiserver_test.go` around lines 29 - 101, Each test (TestGetInvalidSessionSecretsFile, TestGetEmptySessionSecretsFile, TestGetValidSessionSecretsFile) opens a temp file with os.CreateTemp and then calls os.WriteFile on its name without closing the *os.File handle, which can leak resources and cause flaky behavior on some platforms; fix by calling tmpfile.Close() (or defer tmpfile.Close()) immediately after successful os.CreateTemp returns and before calling os.WriteFile/tmpfile.Name(), ensuring the file handle is released before getSessionSecrets reads it.pkg/server/login/login_test.go (1)
248-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t ignore
io.ReadAllerrors in tests.The test discards the error from
io.ReadAll(resp.Body)(data, _ := ...). If reading fails, you’ll get misleading assertion failures.💡 Suggested fix
- data, _ := io.ReadAll(resp.Body) - body := string(data) + data, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("%s: failed to read response body: %v", k, err) + } + body := string(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/login/login_test.go` around lines 248 - 257, The test currently ignores the error returned by io.ReadAll(resp.Body) in the block handling testCase.ExpectContains; change the code to capture and check the error (e.g. data, err := io.ReadAll(resp.Body)), and if err != nil call t.Fatalf or t.Errorf with the error (including context like which test case key `k`) instead of proceeding to string conversion and substring assertions; update references in this block (resp.Body read, variable `body`, and the t.Errorf checks for missing substrings) so assertions only run when the read succeeded.pkg/server/selectprovider/selectprovider_test.go (1)
98-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t ignore
io.ReadAllerrors inselectprovidertest.
data, _ := io.ReadAll(resp.Body)drops the error. If reading fails, yourstrings.Containsassertions may report missing substrings for the wrong reason.💡 Suggested fix
- data, _ := io.ReadAll(resp.Body) - body := string(data) + data, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("%s: failed to read response body: %v", k, err) + } + body := string(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/selectprovider/selectprovider_test.go` around lines 98 - 107, The test is ignoring the error from io.ReadAll(resp.Body) which can mask I/O failures; change the read to capture the error (replace the underscore) and if err != nil call t.Fatalf or t.Errorf (including the test case key `k` and the error) before using `body`, then continue with the existing `strings.Contains` checks that reference `testCase.ExpectContains` and `t.Errorf` for missing substrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/authenticator/password/keystonepassword/keystonepassword_test.go`:
- Around line 61-64: The test HTTP handler currently ignores errors from
io.ReadAll(r.Body) causing json.Unmarshal to mask read failures; change the
handler to capture and assert the read error before unmarshalling (e.g., check
the error returned by io.ReadAll and call th.AssertNoErr or t.Fatalf with that
error and return), so the code around the AuthRequest parsing (variables x
AuthRequest, the io.ReadAll call and subsequent json.Unmarshal) fails fast with
a clear error if the body read fails.
In `@pkg/oauthserver/oauth_apiserver_test.go`:
- Around line 36-38: Update the octal file mode literals in
oauth_apiserver_test.go to use Go's modern form: replace occurrences of
os.FileMode(0600) with os.FileMode(0o600) wherever os.WriteFile is called (e.g.,
the os.WriteFile(tmpfile.Name(), ... , os.FileMode(0600)) invocations around the
tmpfile writes at the three places noted). Keep the same type and semantics,
only change the numeric literal to 0o600 for readability and consistency.
---
Outside diff comments:
In `@pkg/oauthserver/oauth_apiserver_test.go`:
- Around line 29-101: Each test (TestGetInvalidSessionSecretsFile,
TestGetEmptySessionSecretsFile, TestGetValidSessionSecretsFile) opens a temp
file with os.CreateTemp and then calls os.WriteFile on its name without closing
the *os.File handle, which can leak resources and cause flaky behavior on some
platforms; fix by calling tmpfile.Close() (or defer tmpfile.Close()) immediately
after successful os.CreateTemp returns and before calling
os.WriteFile/tmpfile.Name(), ensuring the file handle is released before
getSessionSecrets reads it.
In `@pkg/server/login/login_test.go`:
- Around line 248-257: The test currently ignores the error returned by
io.ReadAll(resp.Body) in the block handling testCase.ExpectContains; change the
code to capture and check the error (e.g. data, err := io.ReadAll(resp.Body)),
and if err != nil call t.Fatalf or t.Errorf with the error (including context
like which test case key `k`) instead of proceeding to string conversion and
substring assertions; update references in this block (resp.Body read, variable
`body`, and the t.Errorf checks for missing substrings) so assertions only run
when the read succeeded.
In `@pkg/server/selectprovider/selectprovider_test.go`:
- Around line 98-107: The test is ignoring the error from io.ReadAll(resp.Body)
which can mask I/O failures; change the read to capture the error (replace the
underscore) and if err != nil call t.Fatalf or t.Errorf (including the test case
key `k` and the error) before using `body`, then continue with the existing
`strings.Contains` checks that reference `testCase.ExpectContains` and
`t.Errorf` for missing substrings.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 86780ec2-73c0-40cb-9007-667113b35dcd
📒 Files selected for processing (9)
pkg/authenticator/password/basicauthpassword/basicauthpassword.gopkg/authenticator/password/keystonepassword/keystonepassword_test.gopkg/config/stringsource.gopkg/oauth/external/gitlab/gitlab_oauth.gopkg/oauth/external/openid/openid.gopkg/oauthserver/auth.gopkg/oauthserver/oauth_apiserver_test.gopkg/server/login/login_test.gopkg/server/selectprovider/selectprovider_test.go
Comment on lines
61
to
64
| var x AuthRequest | ||
| body, _ := ioutil.ReadAll(r.Body) | ||
| body, _ := io.ReadAll(r.Body) | ||
| th.AssertNoErr(t, json.Unmarshal(body, &x)) | ||
| domainName := x.Auth.Identity.Password.User.Domain.Name |
There was a problem hiding this comment.
Handle request-body read errors in the Keystone test handler.
The handler ignores errors from io.ReadAll(r.Body) (body, _ := ...). If that fails, the subsequent json.Unmarshal assertion won’t clearly indicate the underlying read problem.
💡 Suggested fix
- var x AuthRequest
- body, _ := io.ReadAll(r.Body)
- th.AssertNoErr(t, json.Unmarshal(body, &x))
+ var x AuthRequest
+ body, err := io.ReadAll(r.Body)
+ th.AssertNoErr(t, err)
+ th.AssertNoErr(t, json.Unmarshal(body, &x))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/authenticator/password/keystonepassword/keystonepassword_test.go` around
lines 61 - 64, The test HTTP handler currently ignores errors from
io.ReadAll(r.Body) causing json.Unmarshal to mask read failures; change the
handler to capture and assert the read error before unmarshalling (e.g., check
the error returned by io.ReadAll and call th.AssertNoErr or t.Fatalf with that
error and return), so the code around the AuthRequest parsing (variables x
AuthRequest, the io.ReadAll call and subsequent json.Unmarshal) fails fast with
a clear error if the body read fails.
Member
There was a problem hiding this comment.
+1, this seems like a valid addition/fix here.
There was a problem hiding this comment.
@liouk, thanks for the confirmation! Here is the suggested fix for the PR author to apply:
var x AuthRequest
- body, _ := io.ReadAll(r.Body)
- th.AssertNoErr(t, json.Unmarshal(body, &x))
+ body, err := io.ReadAll(r.Body)
+ th.AssertNoErr(t, err)
+ th.AssertNoErr(t, json.Unmarshal(body, &x))This ensures a body-read failure surfaces immediately with a clear error, rather than being silently swallowed and potentially causing a confusing json.Unmarshal failure downstream.
Comment on lines
+36
to
38
| if err := os.WriteFile(tmpfile.Name(), []byte("invalid content"), os.FileMode(0600)); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Use 0o600 instead of 0600 for file mode literals (style/clarity).
These tests use os.FileMode(0600) when writing temp files. Consider switching to os.FileMode(0o600) (Go’s modern octal literal form) for readability/consistency.
Also applies to: 61-63, 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/oauthserver/oauth_apiserver_test.go` around lines 36 - 38, Update the
octal file mode literals in oauth_apiserver_test.go to use Go's modern form:
replace occurrences of os.FileMode(0600) with os.FileMode(0o600) wherever
os.WriteFile is called (e.g., the os.WriteFile(tmpfile.Name(), ... ,
os.FileMode(0600)) invocations around the tmpfile writes at the three places
noted). Keep the same type and semantics, only change the numeric literal to
0o600 for readability and consistency.
Contributor
|
@sebrandon1: This pull request references CNF-23374 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. |
openshift-ci-robot
added
the
jira/valid-reference
Member
Author
|
/retest |
sebrandon1
force-pushed
the
ioutil_deprecation
branch
from
472e50b to
c1dafda
Compare
sebrandon1
force-pushed
the
ioutil_deprecation
branch
from
c1dafda to
b5c492c
Compare
Contributor
|
@sebrandon1: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ioutilhas been deprecated since Go 1.16: https://go.dev/doc/go1.16#ioutilTracking issue: redhat-best-practices-for-k8s/telco-bot#52
Core updates to file and stream reading:
ioutil.ReadAllwithio.ReadAllfor reading from streams and HTTP response bodies across authentication, OAuth, and server-related packages. [1] [2] [3] [4] [5] [6] [7]ioutil.ReadFilewithos.ReadFilefor reading files in configuration and OAuth server logic, such as loading secrets and certificates. [1] [2] [3] [4] [5]Test code modernization:
os.CreateTempinstead ofioutil.TempFileandos.WriteFileinstead ofioutil.WriteFilefor creating and writing temporary files. [1] [2] [3] [4] [5]io.ReadAllinstead ofioutil.ReadAllfor reading HTTP response bodies. [1] [2] [3] [4]General import cleanup:
ioutilimports and added necessaryosorioimports in affected files. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]These updates ensure the codebase is up-to-date with the latest Go standards and removes reliance on deprecated APIs.
Summary by CodeRabbit