Skip to content

fix: Timeout is a duration so we need to specify the correct value#100

Merged
danielpanzella merged 1 commit intomainfrom
danielpanzella/deletion-timeout
Oct 2, 2025
Merged

fix: Timeout is a duration so we need to specify the correct value#100
danielpanzella merged 1 commit intomainfrom
danielpanzella/deletion-timeout

Conversation

@danielpanzella
Copy link
Copy Markdown
Contributor

@danielpanzella danielpanzella commented Oct 1, 2025

Summary by CodeRabbit

  • Refactor
    • Standardized uninstall timeout handling by using explicit time duration units, improving readability and reducing risk of configuration ambiguity. Default timeout remains 10 minutes, and upgrade behavior is unchanged. End-users should experience the same behavior, with improved reliability in edge cases and better consistency across environments. No action is required for existing workflows. This change also lays groundwork for future configurability.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 1, 2025

Walkthrough

Adjusted the Helm uninstall timeout to use a time.Duration by importing time and changing the literal from 600 to 600 * time.Second in pkg/helm/chart.go. No other behavior or APIs changed.

Changes

Cohort / File(s) Summary of Changes
Helm uninstall timeout typing
pkg/helm/chart.go
Imported time; updated Uninstall() timeout literal from 600 to 600 * time.Second to use an explicit time.Duration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Poem

I nudge the clock with a twitch of ear,
From naked 600 to seconds clear.
Ticks align, durations chime,
Helm hops steady, right on time.
In charts I burrow, tidy and neat—
A hare’s small fix makes timing complete. 🐇⏱️

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly conveys that the pull request corrects the timeout value by treating it as a time.Duration, which accurately reflects the change made in pkg/helm/chart.go, making it both specific and concise for reviewers and future reference.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch danielpanzella/deletion-timeout

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c7e220 and 74e5075.

📒 Files selected for processing (1)
  • pkg/helm/chart.go (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-and-scan
  • GitHub Check: Test
  • GitHub Check: Build
🔇 Additional comments (2)
pkg/helm/chart.go (2)

5-5: LGTM!

The time import is necessary for using time.Second in the timeout calculation.


169-169: Critical fix: correct timeout duration.

This change fixes a critical bug where the timeout was effectively 600 nanoseconds (instant timeout) instead of the intended 600 seconds (10 minutes). The Helm uninstall operation now has a proper timeout duration.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@danielpanzella danielpanzella merged commit e6988a3 into main Oct 2, 2025
10 checks passed
@danielpanzella danielpanzella deleted the danielpanzella/deletion-timeout branch October 2, 2025 16:05
jsbroks pushed a commit that referenced this pull request Oct 2, 2025
### [1.21.2](v1.21.1...v1.21.2) (2025-10-02)

### Bug Fixes

* Timeout is a duration so we need to specify the correct value ([#100](#100)) ([e6988a3](e6988a3))
@jsbroks
Copy link
Copy Markdown
Member

jsbroks commented Oct 2, 2025

This PR is included in version 1.21.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants