context: use background() consistently #4801
Conversation
Reviewer's GuideStandardize context usage by replacing all context.TODO() calls with context.Background() across tests, cache, preflight, CLI, compression, repository, and file operations for consistency. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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 |
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
WalkthroughAll instances of Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/os/copy.go (1)
25-27: Pass a caller-supplied context instead of hard-codingcontext.Background()Hard-wiring a root context here prevents higher layers from cancelling long-running sparse copies (for example through a
context.WithTimeout).
Consider changing the API to accept actxparameter and propagating it toCopySparse, falling back tocontext.Background()only whennilis supplied.-func copyFile(src, dst string, sparse bool) error { +func copyFile(ctx context.Context, src, dst string, sparse bool) error { ... - if sparse { - if _, err = CopySparse(context.Background(), out, in); err != nil { + if ctx == nil { + ctx = context.Background() + } + if sparse { + if _, err = CopySparse(ctx, out, in); err != nil {Down-stream helpers (
CopyFile,CopyFileSparse, callers) would need tiny updates to thread the context through.
This keeps the code cancellable without altering behaviour for existing callers.cmd/crc-embedder/cmd/embed.go (1)
162-168: Reuse a single context for the entire download loop
context.Background()is cheap, but constructing it inside the loop makes the intention less explicit and makes later replacement with a cancellable context harder.
Define the context once and reuse it:- for componentName, dl := range downloads { + ctx := context.Background() + for componentName, dl := range downloads { ... - filename, err := download.Download(context.Background(), dl.url, destDir, dl.permissions, nil) + filename, err := download.Download(ctx, dl.url, destDir, dl.permissions, nil)This also opens the door to replacing
ctxwith a timeout or cancel context from the caller in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/crc-embedder/cmd/embed.go(1 hunks)pkg/compress/compress_test.go(1 hunks)pkg/crc/cache/cache.go(2 hunks)pkg/crc/machine/bundle/repository_test.go(1 hunks)pkg/crc/preflight/preflight_checks_common.go(1 hunks)pkg/extract/extract_test.go(2 hunks)pkg/os/copy.go(1 hunks)test/extended/util/util.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/crc-embedder/cmd/embed.go (1)
pkg/download/download.go (1)
Download(64-96)
pkg/compress/compress_test.go (1)
pkg/extract/extract.go (1)
Uncompress(31-33)
pkg/extract/extract_test.go (1)
pkg/extract/extract.go (2)
Uncompress(31-33)UncompressWithFilter(27-29)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-14, 1.23)
🔇 Additional comments (6)
pkg/crc/machine/bundle/repository_test.go (1)
43-44: Consistency change looks goodThe test now uses
context.Background()like the rest of the suite. No further issues spotted.test/extended/util/util.go (1)
145-147: Download now usescontext.Background()– OKThe helper mirrors the updated pattern used across the codebase. No further action required.
pkg/compress/compress_test.go (1)
37-39: Test updated for consistency
extract.Uncompressis now invoked withcontext.Background(). Change is correct and requires no additional adjustments.pkg/crc/preflight/preflight_checks_common.go (1)
128-128: LGTM! Consistent context usage.The replacement of
context.TODO()withcontext.Background()in the bundle download and extraction operations aligns with the PR objective of usingcontext.Background()consistently across the codebase. Both contexts are functionally equivalent, andcontext.Background()is appropriate for these root-level setup operations.Also applies to: 133-133
pkg/extract/extract_test.go (1)
63-63: LGTM! Appropriate context usage in tests.The replacement of
context.TODO()withcontext.Background()in the test functions is correct and follows Go testing best practices. Usingcontext.Background()is standard for test contexts as they represent root-level operations.Also applies to: 151-151, 153-153
pkg/crc/cache/cache.go (1)
125-125: LGTM! Consistent context usage in cache operations.The replacement of
context.TODO()withcontext.Background()in the cache management operations is appropriate. These are infrastructure-level operations for managing the executable cache, and usingcontext.Background()aligns with the PR objective of consistent context usage throughout the codebase.Also applies to: 157-157
|
/ok-to-test |
|
@sebrandon1: 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. |
|
They looks same but have different purpose, |
|
In short you try to say that these are not actually the same, but for our purpose (and the references given) this is not an issue... so the PR is OK. Right? |
3 participants
In the codebase, there are 12 references to context.TODO() and 28 references to context.Background().
They both do the same thing functionally so here's a PR to make it more consistent and change all TODO references to Background.
Similar to:
Summary by Sourcery
Enhancements:
Summary by CodeRabbit