Skip to content

Add timeout flag to repo add and update commands#30900

Merged
gjenkins8 merged 1 commit into
helm:mainfrom
unguiculus:issue-12952
Jun 23, 2025
Merged

Add timeout flag to repo add and update commands#30900
gjenkins8 merged 1 commit into
helm:mainfrom
unguiculus:issue-12952

Conversation

@unguiculus

@unguiculus unguiculus commented May 22, 2025

Copy link
Copy Markdown
Member

What this PR does / why we need it:

Adds --timeout flags to repo add and repo up commands.
Fixes: #12952

Special notes for your reviewer:

This PR supersedes #13185. Adding an environment variable is not ideal IMO. Using CLI flag is more appropriate and consistent with what other commands do.

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 22, 2025
@unguiculus

unguiculus commented May 22, 2025

Copy link
Copy Markdown
Member Author

Why would you want to push this out to Helm 4? IMO this is nothing that needs to wait. It's a non-breaking change. We are really suffering from this.

@robertsirc robertsirc added the v3.x Issues and Pull Requests related to the major version v3 label May 23, 2025
@scottrigby scottrigby added the bug Categorizes issue or PR as related to a bug. label May 23, 2025
scottrigby
scottrigby reviewed May 23, 2025
View reviewed changes

@scottrigby scottrigby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12952 is a bug that affects users when Helm HTTP repos are sufficiently large. To address the error, users should be able to specify a timeout in some way. This PR is a proposal for how to fix that bug.

Process note

Why would you want to push this out to Helm 4? IMO this is nothing that needs to wait. It's a non-breaking change. We are really suffering from this.

Robert is right that this should go into Helm 4 first. That doesn't necessarily mean it should not be backported to Helm 3 though. We are currently in a feature freeze for Helm 3, but we are supporting bug and security fixes according to this plan https://github.com/helm/community/blob/main/hips/hip-0012.md#maintaining-helm-3.

So the process is you should change the base branch of this PR to main (where Helm 4 is being developed). Once merged into main, big fixes can be backported to Helm 3 by opening a new PR against the dev-v3 branch.

@unguiculus

unguiculus commented May 23, 2025

Copy link
Copy Markdown
Member Author

Thanks @scottrigby, I'll rebase the PR.

@unguiculus unguiculus changed the base branch from dev-v3 to main May 23, 2025 17:18
@unguiculus

unguiculus commented May 23, 2025

Copy link
Copy Markdown
Member Author

I rebased and changed the base branch.

@unguiculus unguiculus changed the title Add timeout flag to repo add and update flags Add timeout flag to repo add and update commands May 23, 2025
@unguiculus
Add timeout flag to repo add and update flags
d448cf1 
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>

# Conflicts:
#	pkg/cmd/repo_update.go
scottrigby
scottrigby approved these changes May 27, 2025
View reviewed changes

@scottrigby scottrigby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@scottrigby scottrigby added docs needed Indicates that a PR includes changes that need to be documented. Should not block a PR being merged Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR Has One Approval This PR has one approval. It still needs a second approval to be merged. and removed v3.x Issues and Pull Requests related to the major version v3 labels May 27, 2025
@gjenkins8 gjenkins8 merged commit a2a0935 into helm:main Jun 23, 2025
5 checks passed
@gjenkins8 gjenkins8 removed the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Jun 23, 2025
@unguiculus unguiculus deleted the issue-12952 branch July 10, 2025 08:08
unguiculus added a commit to unguiculus/helm that referenced this pull request Jul 10, 2025
@unguiculus
Add timeout flag to repo add and update flags
79a9cc5 
Backport of helm#30900 to v3.
(cherry picked from commit d448cf1)

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
@unguiculus unguiculus mentioned this pull request Jul 10, 2025
Merged
mattfarina pushed a commit that referenced this pull request Nov 11, 2025
@unguiculus @mattfarina
Add timeout flag to repo add and update flags
a833710 
Backport of #30900 to v3.
(cherry picked from commit d448cf1)

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
(cherry picked from commit 79a9cc5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scottrigby scottrigby scottrigby approved these changes

@gjenkins8 gjenkins8 gjenkins8 approved these changes

Assignees

No one assigned

Labels

bug Categorizes issue or PR as related to a bug. docs needed Indicates that a PR includes changes that need to be documented. Should not block a PR being merged Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Command "repository add" raises "Client.Timeout or context cancellation while reading body"

4 participants

@unguiculus @scottrigby @gjenkins8 @robertsirc