Skip to content

chore: DefaultHTTPTimeout can be configured via the environment variable#13185

Closed
0xff-dev wants to merge 1 commit into
helm:mainfrom
0xff-dev:main
Closed

chore: DefaultHTTPTimeout can be configured via the environment variable#13185
0xff-dev wants to merge 1 commit into
helm:mainfrom
0xff-dev:main

Conversation

@0xff-dev

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
The previous PR #12203, which added a default timeout for httpProvider, was not configurable, and commands such as helm repo add, helm repo update, etc. did not have a timeout parameter, and there was a problem with Issue #12952.

To solve this problem, there are two possible options

  1. add a timeout parameter to the helm repo subcommands.
  2. make DefaultHTTPTimeout configurable via environment variables.

Considering the minimum changes and impact, use option 2.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@pull-request-size pull-request-size Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 17, 2024
Comment thread pkg/getter/getter.go Outdated
Comment thread pkg/getter/getter.go Outdated
Signed-off-by: 0xff-dev <stevenshuang521@gmail.com>
Comment thread pkg/getter/getter.go
// https://github.com/curl/curl/blob/master/lib/connect.h#L40C21-L40C21
// The helm commands are usually executed manually. Considering the acceptable waiting time, we reduced the entire request time to 120s.
DefaultHTTPTimeout = 120
defaultHTTPTimeout = 120

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultHTTPTimeout = 120
DefaultHTTPTimeout = 120

Since this is not an internal package, this probably needs to stay public or this would be a breaking change.

Comment thread pkg/getter/getter.go
defaultHTTPTimeout = 120
)

var defaultOptions = []Option{WithTimeout(time.Second * DefaultHTTPTimeout)}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure you want to makeGetDfaultHTTPTimeout and DefaultOptions public unless there is a need.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My previous idea was that if a user's project depends on helm and wants to get the default http timeout, helm should provide a method to get it, so I set the GetDefaultHTTPTimeout method to public.
And there is no need to set the DefaultOptions method to public, I'll Update it.

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale because it has been open for 90 days with no activity. This pull request will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions Bot added the Stale label Aug 20, 2025
@0xff-dev 0xff-dev closed this Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files. Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants