🌱 move BuildHTTPClient from shared httputil to catalogmetadata/client#2782
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the operator-controller’s catalog metadata HTTP client behavior to better accommodate large catalog responses from catalogd by aligning the client timeout with catalogd’s longer write timeout, and relocates the HTTP client construction helper into the catalog metadata client package where it’s actually used.
Changes:
- Increase the catalog metadata HTTP client timeout from 10s to 5m.
- Move
BuildHTTPClientintointernal/operator-controller/catalogmetadata/clientand update call sites accordingly. - Update unit tests and imports to reference the new
BuildHTTPClientlocation/package.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/operator-controller/catalogmetadata/client/httputil.go | Defines BuildHTTPClient in the catalog metadata client package and sets the timeout to 5 minutes. |
| internal/operator-controller/catalogmetadata/client/httputil_test.go | Updates tests to call BuildHTTPClient from the catalog metadata client package. |
| internal/operator-controller/catalogmetadata/client/client_test.go | Renames import alias/type references to the catalog metadata client package consistently. |
| cmd/operator-controller/main.go | Switches the operator-controller wiring to call catalogclient.BuildHTTPClient instead of the shared util function. |
Comments suppressed due to low confidence (1)
internal/operator-controller/catalogmetadata/client/httputil_test.go:119
- The PR’s main behavioral change is increasing the HTTP client timeout to 5 minutes, but the tests don’t assert the configured timeout. Adding an assertion here would prevent accidental regression back to a shorter timeout.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
+ Coverage 70.40% 70.50% +0.09%
==========================================
Files 143 143
Lines 10617 10617
==========================================
+ Hits 7475 7485 +10
+ Misses 2580 2573 -7
+ Partials 562 559 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
I would focus this PR solely on the timeout increase, and rest of refactoring would push into another PR.
| func BuildHTTPClient(cpw *CertPoolWatcher) (*http.Client, error) { | ||
| httpClient := &http.Client{Timeout: 10 * time.Second} | ||
| func BuildHTTPClient(cpw *httputil.CertPoolWatcher) (*http.Client, error) { | ||
| httpClient := &http.Client{Timeout: 5 * time.Minute} |
There was a problem hiding this comment.
can we extract it into a constant?
There was a problem hiding this comment.
I'll split it up.
On the constant, I don't know that we should have one constant shared between the client and server. The server's timeout needs to consider all of its potential client use cases more broadly than just the op-con client. The reason they were different before is that catd needed a longer timeout to accommodate off-cluster clients, which could theoretically be on high-latency/low-bandwidth connections).
We could bump the op-con client timeout to something between 10s and 5m (e.g. like 2m), but that seems arbitrary, and 5m doesn't seem excessively long for op-con anyway. If we bump catalogd up again later (e.g. to 20m), I don't think we'd automatically want to bump op-con client to that as well, would we?
I could extract separate constants for the existing values, but 🤷♂️ , maybe not necessary if there's only one place they're used. Not opinionated either way on this.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
joelanford
force-pushed
the
move-httpclient-to-catalogclient
branch
from
812a288 to
51aa279
Compare
joelanford
changed the title
|
Updated this PR to be just the refactoring. I'll make a follow-up for bumping the timeout. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
openshift-merge-bot
Bot
merged commit f5e0d77
into
operator-framework:main
Description
Move
BuildHTTPClientfrominternal/shared/util/httpintointernal/operator-controller/catalogmetadata/client, since it is onlyused by the catalog client. This reduces the surface of the shared
httputil package and co-locates the HTTP client construction with its
sole consumer.
httputiltocatalogclientinmain.goand testshttputil.goandhttputil_test.gointo theclientpackageReviewer Checklist