feat: add deadline instead of timeout #599
Conversation
WalkthroughThe pull request introduces a significant change in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Nitpick comments (3)
pkg/knuu/knuu.go (3)
28-31: Consider making these constants private or grouping them.They appear to be used only in the local package. If they are strictly internal constants, consider making them unexported (e.g., lowercase) or grouping them to make them easier to discover and maintain.
51-52: Mark deprecated fields clearly and remove them in future releases.You have introduced a new Deadline field while keeping Timeout field for backward compatibility. Consider using Go’s official deprecation comment style (e.g. “// Deprecated: Use Deadline instead.”) to help IDEs and linters.
114-116: Double-check the immediate usage of time.Now() in cleanup.You are calling handleDeadline with time.Now() as the deadline. This effectively triggers cleanup operations immediately. If the intent is to attempt graceful termination for a short interval, you may wish to add a small buffer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/sidecars/suite_setup_test.go(1 hunks)pkg/knuu/knuu.go(6 hunks)pkg/knuu/knuu_test.go(1 hunks)
🔇 Additional comments (4)
pkg/knuu/knuu.go (3)
78-85: Ensure clarity around fallback logic for deadline vs. deprecated timeout.
When opts.Deadline is zero, the code logs a warning upon using Timeout but gracefully sets the Deadline if Timeout is present. This is a good approach, but ensure you remove the Timeout usage in a future major release to avoid confusion.
88-88: Confirm error handling for handleDeadline.
Currently, if handleDeadline returns an error, the constructor wraps it and returns. Confirm that the calling contexts handle (or log) this error sufficiently, since it potentially prevents the entire environment setup.
Line range hint 125-145: Validate that the command sequence will not block or fail silently.
handleDeadline builds a multi-step shell command to delete namespace resources. Consider logging each step to facilitate debugging in case of partial resource deletions. Also, verify that all potential errors raised by each chained command are handled.
e2e/sidecars/suite_setup_test.go (1)
37-38: Good adaptation to deadline-based approach.
Replacing Timeout with Deadline conforms with the new approach. Make sure to test and confirm that the new deadline-based behavior works as intended within the CI pipeline timelines.
| @@ -74,7 +74,7 @@ func TestNew(t *testing.T) { | |||
| assert.NotNil(t, k.K8sClient) | |||
| assert.NotNil(t, k.ImageBuilder) | |||
| assert.NotEmpty(t, k.Scope) | |||
| assert.Equal(t, defaultTimeout, defaultTimeout, timeoutHandlerName) | |||
| assert.Equal(t, defaultTimeout, defaultTimeout, deadlineHandlerName) | |||
There was a problem hiding this comment.
Revisit the usage of assert.Equal here.
The current invocation, assert.Equal(t, defaultTimeout, defaultTimeout, deadlineHandlerName), compares defaultTimeout with itself and uses deadlineHandlerName as the message. This has no real assertion effect. Consider adjusting it if your goal was to test that the defaultTimeout is applied or to ensure that the correct handler name is used.
- assert.Equal(t, defaultTimeout, defaultTimeout, deadlineHandlerName)
+ // Example fix if validating actual usage of defaultTimeout and handler name
+ assert.Equal(t, defaultTimeout, someComputedTimeout, "defaultTimeout should match the expected fallback")
+ assert.Equal(t, "deadline-handler", deadlineHandlerName, "Invalid or missing handler name")Committable suggestion skipped: line range outside the PR's diff.
Closes #367
Summary by CodeRabbit
New Features
Deadlineoption for configuration.Bug Fixes
Documentation
Timeoutfield in favor of the newDeadlinefield.