NO-JIRA: Replace deprecated io/ioutil pkg #15902
Conversation
jhadvig
changed the title
|
@jhadvig: 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
WalkthroughThis pull request systematically migrates the codebase from the deprecated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig 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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/chartproxy/repos.go (1)
83-93: Adddefer resp.Body.Close()to prevent resource leak.The HTTP response body from
httpClient.Get()is never closed, causing a resource leak that can lead to connection pool exhaustion and memory leaks over time.🔧 Proposed fix to close the response body
resp, err := httpClient.Get(indexURL) if err != nil { return nil, err } + defer resp.Body.Close() if resp.StatusCode != 200 { return nil, errors.New(fmt.Sprintf("Response for %v returned %v with status code %v", indexURL, resp, resp.StatusCode)) } body, err := io.ReadAll(resp.Body)
🤖 Fix all issues with AI agents
In @pkg/helm/actions/auth_test.go:
- Around line 3-6: The test currently calls os.ReadFile for "./server.crt" and
"./server.key" but ignores the returned errors; change these calls to check the
error and fail the test immediately (use t.Fatalf or t.Fatal) when os.ReadFile
returns a non-nil error so the test stops with a clear message about failing to
read the certificate/key (refer to the variables holding the cert/key bytes and
the test function in pkg/helm/actions/auth_test.go where the reads occur, lines
around the existing os.ReadFile calls).
🧹 Nitpick comments (3)
pkg/helm/actions/setup_test.go (1)
88-105: Consider capturing command stderr to improve error messages.The current implementation redirects the command's stderr to
os.Stderr(line 91), then attempts to read fromos.Stderrin error handlers (lines 94, 100). This won't work as intended becauseos.Stderris a write-only stream for the current process—you can't read back output that's already been written.To include actual stderr content in error messages, capture it to a buffer instead.
♻️ Proposed fix to capture stderr
func ExecuteScript(filepath string, waitForCompletion bool) error { + var stderr bytes.Buffer tlsCmd := exec.Command(filepath) tlsCmd.Stdout = os.Stdout - tlsCmd.Stderr = os.Stderr + tlsCmd.Stderr = &stderr err := tlsCmd.Start() if err != nil { - bytes, _ := io.ReadAll(os.Stderr) - return fmt.Errorf("Error starting program :%s:%s:%w", filepath, string(bytes), err) + return fmt.Errorf("Error starting program :%s:%s:%w", filepath, stderr.String(), err) } if waitForCompletion { err = tlsCmd.Wait() if err != nil { - bytes, _ := io.ReadAll(os.Stderr) - return fmt.Errorf("Error waiting program :%s:%s:%w", filepath, string(bytes), err) + return fmt.Errorf("Error waiting program :%s:%s:%w", filepath, stderr.String(), err) } } return nil }Add
bytesto imports:import ( "bytes" "fmt" "io" "os" "os/exec" "testing" "time" "helm.sh/helm/v3/pkg/cli" )pkg/helm/chartproxy/repos_test.go (1)
633-634: API replacement is correct; however, consider refactoring the stderr handling.The replacement of
ioutil.ReadAllwithio.ReadAllis correct. However, there's a pre-existing issue with the error handling logic: attempting to read fromos.Stderrdoesn't work as intended because it's the process's own standard error stream, not the command's output. SincetlsCmd.Stderr = os.Stderr(line 630), command errors are already being written directly to the process's stderr, making this read operation ineffective (it would return empty bytes or fail).If you want to capture the command's stderr for inclusion in error messages, consider using a
bytes.Buffer:♻️ Proposed refactor to capture command stderr
func ExecuteScript(filepath string, waitForCompletion bool) error { tlsCmd := exec.Command(filepath) + var stderrBuf bytes.Buffer tlsCmd.Stdout = os.Stdout - tlsCmd.Stderr = os.Stderr + tlsCmd.Stderr = io.MultiWriter(os.Stderr, &stderrBuf) err := tlsCmd.Start() if err != nil { - bytes, _ := io.ReadAll(os.Stderr) - return fmt.Errorf("Error starting command :%s:%s:%w", filepath, string(bytes), err) + return fmt.Errorf("Error starting command :%s:%s:%w", filepath, stderrBuf.String(), err) } if waitForCompletion { err = tlsCmd.Wait() if err != nil { - bytes, _ := io.ReadAll(os.Stderr) - return fmt.Errorf("Error waiting command :%s:%s:%w", filepath, string(bytes), err) + return fmt.Errorf("Error waiting command :%s:%s:%w", filepath, stderrBuf.String(), err) } } return nil }This captures stderr in a buffer while still writing it to the console, allowing the error messages to include actual command output.
Also applies to: 639-640
pkg/terminal/proxy.go (1)
239-255:io.ReadAllreplacement is correct; consider dropping redundantio.NopCloserhere.
http.NewRequestwill wrap anio.Readerbody withio.NopCloserautomatically.Proposed tweak
- wkspReq, err := http.NewRequest(http.MethodPost, host.String(), io.NopCloser(bytes.NewReader(body))) + wkspReq, err := http.NewRequest(http.MethodPost, host.String(), bytes.NewReader(body))
📜 Review details
Configuration used: Organization 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 (22)
pkg/devfile/sample_test.gopkg/graphql/resolver/k8s.gopkg/helm/actions/auth_test.gopkg/helm/actions/get_chart_test.gopkg/helm/actions/get_release_test.gopkg/helm/actions/install_chart_test.gopkg/helm/actions/list_releases_test.gopkg/helm/actions/release_history_test.gopkg/helm/actions/rollback_release_test.gopkg/helm/actions/setup_test.gopkg/helm/actions/template_test.gopkg/helm/actions/uninstall_release_test.gopkg/helm/actions/upgrade_release_test.gopkg/helm/chartproxy/proxy_test.gopkg/helm/chartproxy/repos.gopkg/helm/chartproxy/repos_test.gopkg/helm/chartverifier/chart_verifier_test.gopkg/helm/metrics/metrics_test.gopkg/proxy/proxy_test.gopkg/server/certs.gopkg/serverconfig/config.gopkg/terminal/proxy.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:
pkg/helm/actions/list_releases_test.gopkg/graphql/resolver/k8s.gopkg/serverconfig/config.gopkg/helm/actions/upgrade_release_test.gopkg/helm/actions/auth_test.gopkg/helm/actions/get_chart_test.gopkg/proxy/proxy_test.gopkg/helm/actions/get_release_test.gopkg/helm/actions/setup_test.gopkg/terminal/proxy.gopkg/helm/metrics/metrics_test.gopkg/helm/actions/release_history_test.gopkg/devfile/sample_test.gopkg/helm/chartproxy/repos.gopkg/helm/chartproxy/repos_test.gopkg/helm/actions/template_test.gopkg/helm/chartproxy/proxy_test.gopkg/helm/actions/rollback_release_test.gopkg/helm/actions/uninstall_release_test.gopkg/helm/actions/install_chart_test.gopkg/helm/chartverifier/chart_verifier_test.gopkg/server/certs.go
🔇 Additional comments (34)
pkg/helm/chartverifier/chart_verifier_test.go (2)
5-5: LGTM! Correct migration from deprecated package.The import change from
io/ioutiltoiocorrectly follows Go's deprecation guidance (Go 1.16+).
31-31: LGTM! Correct usage of the new API.Both occurrences correctly replace
ioutil.Discardwithio.Discard, maintaining identical functionality while using the current standard library API.Also applies to: 48-48
pkg/helm/actions/uninstall_release_test.go (1)
6-6: LGTM! Clean migration from deprecated io/ioutil.The replacement of
ioutil.Discardwithio.Discardis correct and consistent across all test functions. This migration aligns with Go 1.16+ best practices and has no impact on test behavior.Also applies to: 41-41, 78-78, 124-124, 174-174
pkg/helm/actions/template_test.go (1)
5-5: LGTM! Clean migration from deprecatedio/ioutil.The changes correctly replace deprecated
ioutil.Discardwithio.Discardand update the import accordingly. The functionality remains identical and improves maintainability.Also applies to: 99-99, 185-185
pkg/helm/actions/rollback_release_test.go (2)
6-6: LGTM! Correct migration from deprecated package.The import change from
io/ioutiltoiofollows Go's deprecation guidance (deprecated since Go 1.16).
55-55: LGTM! Correct replacement of deprecated API.Both occurrences correctly replace
ioutil.Discardwithio.Discard, preserving the exact same behavior for discarding KubeClient output in test setup.Also applies to: 100-100
pkg/proxy/proxy_test.go (2)
4-4: LGTM! Correct import migration.The import change from
io/ioutiltoioaligns with Go 1.16+ best practices for moving away from the deprecatedioutilpackage.
56-56: LGTM! Correct function migration.The replacement of
ioutil.ReadAllwithio.ReadAllis the proper migration path. The function signatures are identical, ensuring no behavior change.pkg/helm/actions/setup_test.go (1)
5-5: LGTM! Correct deprecation migration.The import change from
io/ioutiltoiocorrectly reflects the migration ofReadAllto theiopackage in Go 1.16+.pkg/helm/chartproxy/repos.go (1)
9-9: LGTM! Migration from deprecated package is correct.The changes properly migrate from the deprecated
io/ioutilpackage to the standardiopackage. Both the import update and theio.ReadAllfunction call replacement are correct and maintain identical behavior.Also applies to: 90-90
pkg/helm/chartproxy/repos_test.go (2)
6-6: LGTM! Import and API update are correct.The addition of the
ioimport and the replacement ofioutil.NopCloserwithio.NopClosercorrectly follows Go's deprecation migration path for theio/ioutilpackage.Also applies to: 378-378
395-395: LGTM! File reading API correctly updated.The replacements of
ioutil.ReadFilewithos.ReadFileare correct and properly follow Go's deprecation guidelines. Error handling for file operations remains appropriate.Also applies to: 598-600
pkg/helm/actions/list_releases_test.go (2)
3-15: Good stdlib migration (ioutil.Discard→io.Discard).This keeps the test behavior the same while removing deprecated
io/ioutilusage.Also applies to: 45-50
3-5: Go version (1.24.0) fully supportsio.Discardandos.ReadFile— no compatibility concerns.The codebase targets Go 1.24.0, which exceeds the Go 1.16+ requirement. The file successfully uses
io.Discardat line 47. No action needed.pkg/server/certs.go (1)
3-16: Correct replacement (ioutil.ReadFile→os.ReadFile).pkg/serverconfig/config.go (1)
3-12: Correct replacement (ioutil.ReadFile→os.ReadFile) with no behavioral change.Also applies to: 137-143
pkg/helm/metrics/metrics_test.go (1)
3-14: Correct replacement (ioutil.ReadAll→io.ReadAll) with same test semantics.Also applies to: 151-160
pkg/devfile/sample_test.go (2)
3-15: Import change looks correct (switch toos).
138-149:os.ReadFileswap is correct and keeps behavior the same.pkg/helm/chartproxy/proxy_test.go (3)
3-16: Import update toosis correct forReadFileusage.
120-129:os.ReadFileuse for loading index testdata is correct.
163-172:os.ReadFileuse for loading expected merged index is correct.pkg/helm/actions/release_history_test.go (3)
3-14: Import update toiois correct forio.Discard.
41-46:io.Discardreplacement is correct (same sink semantics).
97-103:io.Discardreplacement is correct in the non-exist history test as well.pkg/terminal/proxy.go (1)
3-13: Import update is consistent withio.ReadAll/io.NopCloserusage.pkg/graphql/resolver/k8s.go (4)
3-16: Import update toiois correct forio.ReadAll.
22-41:io.ReadAllreplacement inFetchURLis correct and preserves behavior.
100-106:io.ReadAllreplacement in error-body handling is correct.
145-151:io.ReadAllreplacement in SSAR error-body handling is correct.pkg/helm/actions/install_chart_test.go (1)
5-6: LGTM! Clean migration from deprecated ioutil.The migration from
io/ioutiltoioandospackages is correct and follows Go 1.16+ best practices. All replacements are functionally equivalent:
ioutil.Discard→io.Discardioutil.ReadFile→os.ReadFileAlso applies to: 98-98, 177-177, 188-188, 190-190, 201-201, 274-274, 297-297, 361-361
pkg/helm/actions/upgrade_release_test.go (1)
5-6: LGTM! Consistent migration pattern.All
ioutilusages correctly replaced with modern equivalents across multiple test functions.Also applies to: 107-107, 136-136, 138-138, 214-214, 282-282, 344-344, 456-456, 485-485, 487-487, 575-575, 667-667
pkg/helm/actions/get_release_test.go (1)
5-6: LGTM! Correct API migration.All deprecated
ioutilfunctions properly replaced with their modern counterparts.Also applies to: 74-74, 159-159, 170-170, 172-172, 183-183
pkg/helm/actions/get_chart_test.go (1)
4-5: LGTM! Complete and correct migration.All
ioutilusages consistently migrated toioandospackages across test cases.Also applies to: 93-93, 206-206, 220-220, 222-222, 233-233, 340-340
| import ( | ||
| "io/ioutil" | ||
| "os" | ||
| "testing" | ||
|
|
There was a problem hiding this comment.
Don’t ignore os.ReadFile errors in tests.
If ./server.crt / ./server.key can’t be read, the test will proceed with empty/nil data and fail later in less obvious ways.
Proposed fix
if tt.hasTLS {
- cert, _ := os.ReadFile("./server.crt")
- key, _ := os.ReadFile("./server.key")
+ cert, err := os.ReadFile("./server.crt")
+ require.NoError(t, err)
+ key, err := os.ReadFile("./server.key")
+ require.NoError(t, err)
tlsSecret := &v1.Secret{
Data: map[string][]byte{
tlsSecretCertKey: cert,
tlsSecretKey: key,
},
ObjectMeta: metav1.ObjectMeta{Name: "test-tls", Namespace: configNamespace},
}
objs = append(objs, tlsSecret)
}Also applies to: 71-82
🤖 Prompt for AI Agents
In @pkg/helm/actions/auth_test.go around lines 3 - 6, The test currently calls
os.ReadFile for "./server.crt" and "./server.key" but ignores the returned
errors; change these calls to check the error and fail the test immediately (use
t.Fatalf or t.Fatal) when os.ReadFile returns a non-nil error so the test stops
with a clear message about failing to read the certificate/key (refer to the
variables holding the cert/key bytes and the test function in
pkg/helm/actions/auth_test.go where the reads occur, lines around the existing
os.ReadFile calls).
Leo6Leo
left a comment
Leo6Leo
left a comment
There was a problem hiding this comment.
This file get missed:
https://github.com/jhadvig/console/blob/ioutil/cmd/bridge/main.go
|
/retest-required |
|
@jhadvig: 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. |
3 participants
/assign @Leo6Leo