Replace RestClient with Faraday 2.x for persistent Candlepin connections#11754
Open
pablomh wants to merge 36 commits into
Open
Replace RestClient with Faraday 2.x for persistent Candlepin connections#11754pablomh wants to merge 36 commits into
pablomh wants to merge 36 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/katello/resources/candlepin.rb (1)
152-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the upstream port in
site.
upstream_api_uri.portis discarded here, so any non-default CDN/Candlepin endpoint silently gets rewritten to 80/443.Suggested fix
def site - "#{upstream_api_uri.scheme}://#{upstream_api_uri.host}" + "#{upstream_api_uri.scheme}://#{upstream_api_uri.host}:#{upstream_api_uri.port}" end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/resources/candlepin.rb` around lines 152 - 154, The site method currently builds the base URL from upstream_api_uri.scheme and .host but drops upstream_api_uri.port, causing non-default ports to be lost; update the site method (def site) to append the port when upstream_api_uri.port is present and not the default for the scheme (e.g., include ":#{upstream_api_uri.port}" when upstream_api_uri.port exists and isn’t 80 for http or 443 for https) so the returned URI preserves custom CDN/Candlepin endpoints.
🧹 Nitpick comments (3)
test/lib/http_resource_test.rb (1)
30-40: ⚡ Quick winAdd reserved-character encoding coverage in query-string tests.
These cases validate structure/order, but not escaping of values like spaces,
&, or=. A small extra case would make serialization regressions much easier to catch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/http_resource_test.rb` around lines 30 - 40, Add a new unit test in test/lib/http_resource_test.rb that calls TestHttpResource.query_string with parameters containing reserved characters (e.g., values with spaces, ampersands, and equals like "a b", "c&d", "e=f") to assert they are percent-encoded in the serialized query string; create a method such as test_query_string_reserved_characters that passes either a Hash or array-of-tuples matching the existing ordering expectations and asserts the expected encoded output (e.g., space -> %20, & -> %26, = -> %3D) to catch regression in TestHttpResource.query_string encoding behavior.app/lib/actions/candlepin/environment/set_content.rb (1)
34-47: ⚡ Quick winUse
casefor status branching in the add-content rescue block.This
if/elsifpattern on a single discriminator (e.code) triggersStyle/CaseLikeIfand is easier to read ascase.♻️ Suggested refactor
- if e.code == '409' + case e.code + when '409' # Candlepin raises a 409 in case it gets a duplicate content id add to an environment. # Refresh the existing ids list and try again. raise e if ((retries += 1) == max_retries) output[:add_ids] = content_ids - existing_ids - elsif e.code == '404' + when '404' # Set a higher limit for retries just in case the missing content is not being parsed correctly. # If the content is not found after the retries, assume it is gone and continue. raise e if ((retries += 1) == 1_000) missing_content = e.message.split(' ')[-1].gsub(/[".]/, '') Rails.logger.debug "Content #{missing_content} not found in the environment. Removing it from the add_ids list." output[:add_ids].delete(missing_content) else raise end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/actions/candlepin/environment/set_content.rb` around lines 34 - 47, Replace the if/elsif chain that branches on e.code inside the add-content rescue block with a case statement (case e.code; when '409' ... when '404' ... else ... end) to satisfy Style/CaseLikeIf while preserving existing behavior: on '409' keep the retries increment and raise when (retries += 1) == max_retries and refresh output[:add_ids] = content_ids - existing_ids; on '404' keep the higher retry threshold (raise when (retries += 1) == 1_000), compute missing_content the same way, log the debug message, and remove it from output[:add_ids]; otherwise re-raise the exception. Ensure variables and side-effects (retries, max_retries, output[:add_ids], content_ids, existing_ids, and the Rails.logger.debug call) remain unchanged.app/controllers/katello/api/registry/registry_proxies_controller.rb (1)
48-50: ⚡ Quick winKeep
BLOB_UNKNOWN404s out of error logs.The
RestClient::Exceptionbranch right above intentionally downgrades404 BLOB_UNKNOWNlookups toinfobecause podman push triggers them routinely. This newHttpErrorpath logs every such miss aserror, so once registry requests come throughHttpErrorthe logs will start filling with expected failures again.Proposed fix
rescue_from HttpResource::HttpError do |e| - Rails.logger.error(pp_exception(e, with_backtrace: false)) + if e.code.to_i == 404 + parsed = JSON.parse(e.response_body.to_s) rescue {} + if parsed["errors"]&.any? { |error| error["code"] == "BLOB_UNKNOWN" } + Rails.logger.info(pp_exception(e, with_backtrace: false)) + else + Rails.logger.error(pp_exception(e, with_backtrace: false)) + end + else + Rails.logger.error(pp_exception(e, with_backtrace: false)) + end body = e.response_body || e.message🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/katello/api/registry/registry_proxies_controller.rb` around lines 48 - 50, The HttpResource::HttpError rescue handler (rescue_from HttpResource::HttpError do |e|) currently logs all misses as errors; change it to detect a 404 response (e.response_code == 404 or e.response_body includes "BLOB_UNKNOWN") and log those cases with Rails.logger.info (or downgrade the log level) instead of Rails.logger.error, preserving the existing body = e.response_body || e.message assignment and only calling Rails.logger.error for non-404/ non-BLOB_UNKNOWN errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/katello/api/registry/registry_proxies_controller.rb`:
- Around line 50-54: Replace the current fallback logic that sets body =
e.response_body || e.message with a presence-aware check so empty strings from
Candlepin don't override the fallback: use e.response_body.presence || e.message
when building body (in the RegistryProxiesController error handling block where
body is assigned and rendered for request_from_katello_cli? / render plain
paths) so blank response_body values fall back to e.message.
In `@app/lib/actions/middleware/propagate_candlepin_errors.rb`:
- Around line 20-26: The rescue currently converts every HttpResource::HttpError
into Katello::Errors::CandlepinError, losing upstream-specific types; change the
re-raise to preserve upstream error classes: after extracting display_message
(using JSON.parse of e.response_body as done), check if
e.is_a?(Katello::Errors::UpstreamCandlepinError) (or whatever upstream subclass
lives under Katello::Errors) and if so re-raise an instance of e.class with the
display_message (e.g. raise e.class, display_message, e.backtrace) so downstream
rescues still match, otherwise raise ::Katello::Errors::CandlepinError with
display_message as today.
In `@app/lib/katello/http_resource.rb`:
- Around line 64-77: The rescue block in http_resource.rb currently converts
parse failures for many HTTP error statuses into NetworkException which breaks
callers expecting HttpError; adjust the rescue so that when resp is present
(i.e. an HTTP response was received) you always raise HttpError with the
existing payload ({:message => message_or_error, :service_code => service_code,
:code => status_code, :response_body => resp.body}, caller) instead of raising
NetworkException for non-JSON bodies—preserve the existing special-case logging
for 404/500/502/503/504 if desired, but ensure 401/409/410 and other HTTP error
statuses still raise HttpError; use the existing locals resp.status, error (from
rescue), message, service_code and status_code to build the HttpError.
In `@app/lib/katello/resources/candlepin.rb`:
- Around line 167-174: The method upstream_owner_id is bypassing normal HTTP
error handling by passing process: false to
Katello::Resources::Candlepin::UpstreamConsumer.issue_request; remove the
process: false option so issue_request will raise the appropriate HTTP
exceptions on 401/404/410 instead of returning a raw response, then parse
JSON.parse(response.body)['owner']['key'] only after a successful call; update
the call site in upstream_owner_id (and any similar callers) to rely on the
default processing behavior of issue_request.
In `@app/lib/katello/resources/candlepin/activation_key.rb`:
- Around line 55-67: The JSON.parse call is using the response object itself
instead of its body after the RestClient→Faraday change; update the tail of the
method so it parses the response body when result is a Faraday/response object
(e.g. if result.respond_to?(:body) use result.body, otherwise fall back to
result or '{}'), then pass the parsed value into
::Katello::Util::Data.array_with_indifferent_access; adjust references around
Candlepin::CandlepinResource.put and Candlepin::CandlepinResource.issue_request
to use the chosen result-body extraction.
In `@app/lib/katello/resources/candlepin/consumer.rb`:
- Around line 47-52: In async_hypervisors, self.post now returns an HTTP
response wrapper, so JSON.parse is being called on the response object instead
of the body; update the method (async_hypervisors) to extract the response body
(e.g., response.body) and parse that, then call with_indifferent_access on the
parsed object; also consider handling a nil/empty body before parsing to avoid
exceptions.
- Around line 138-150: The code currently calls JSON.parse(result) but result is
the HTTP response object returned by
Candlepin::CandlepinResource.put/issue_request; parse the response body string
instead (e.g., result.body or result.read_body depending on the client) before
passing it to JSON.parse and then to
::Katello::Util::Data.array_with_indifferent_access, and ensure result is set
(or handle nil) when neither attrs_to_update nor attrs_to_delete runs; update
references to Candlepin::CandlepinResource.put, issue_request, the local result
variable, and the final ::Katello::Util::Data.array_with_indifferent_access call
accordingly.
In `@app/lib/katello/resources/candlepin/owner.rb`:
- Around line 80-86: The destroy_imports method is parsing Faraday response
objects directly (response_json = self.delete(...) and response_json =
self.get(...)) which causes JSON.parse errors; change the code to extract the
response body (use response.body) before calling JSON.parse in both the delete
and get calls so that JSON.parse receives a string; update the variables
referenced in destroy_imports (response_json and response) accordingly and
preserve the existing with_indifferent_access handling.
In `@app/lib/katello/resources/candlepin/upstream_pool.rb`:
- Around line 8-17: The get method in upstream_pool.rb currently calls
issue_request with process: false and only checks for response.status == 410,
which lets other non-2xx responses slip through; update the get method (the
issue_request call and its immediate response handling) to explicitly guard
non-2xx statuses before returning: if response.status == 410 raise
Katello::Errors::UpstreamConsumerGone, if response.status == 401 (if your
upstream-consumer contract requires) map to the same or another appropriate
Katello::Errors class, and for any other non-2xx status raise a
generic/error-wrapping exception (include status and response body) so
downstream parsing won’t receive raw error responses. Ensure you still pass
default_headers and params when reusing issue_request and only return the
response for 2xx statuses.
In `@app/services/katello/registration_manager.rb`:
- Around line 294-298: Replace the if/elsif chain that checks e.code with a case
statement: change the conditional starting with "if e.code == '404'" (which
warns about missing consumer) and the "elsif e.code == '410'" branch (which
warns about already removed consumer) to "case e.code; when '404' ... when '410'
... else ... end", keeping the same Rails.logger.warn(...) calls and the
host_uuid reference so behavior is unchanged while satisfying Style/CaseLikeIf.
In `@test/lib/http_resource_test.rb`:
- Around line 47-49: The test currently only asserts that `@mock_conn.send` was
invoked and yields a permissive stub_request, which lets regressions in
header/body assignment slip by; update the tests (e.g., the case using
`@mock_conn.expects`(:send).with(:get,
'/path').yields(stub_request).returns(`@mock_response`) and the other similar
cases) to set stricter expectations on the yielded stub_request object
itself—expect calls like
stub_request.expects(:headers=).with(hash_including('headerOne' =>
'headerOneValue')) and/or stub_request.expects(:body=).with(expected_body) (or
use assert_received/assert_called equivalents) before invoking
TestHttpResource.get to ensure headers/body are actually assigned rather than
only verifying send was called.
In `@test/support/scenario_support.rb`:
- Around line 75-76: In the rescue block inside exists? that currently does
"rescue HttpResource::HttpError => e; raise unless e.code == '404'", change the
comparison to a numeric one so both string and integer codes match (e.g. use
e.code.to_i == 404 or Integer(e.code) == 404 rescue nil) and re-raise only when
the numeric comparison is not 404; keep the rescue clause (rescue
HttpResource::HttpError => e) and ensure you still re-raise for non-404
responses.
---
Outside diff comments:
In `@app/lib/katello/resources/candlepin.rb`:
- Around line 152-154: The site method currently builds the base URL from
upstream_api_uri.scheme and .host but drops upstream_api_uri.port, causing
non-default ports to be lost; update the site method (def site) to append the
port when upstream_api_uri.port is present and not the default for the scheme
(e.g., include ":#{upstream_api_uri.port}" when upstream_api_uri.port exists and
isn’t 80 for http or 443 for https) so the returned URI preserves custom
CDN/Candlepin endpoints.
---
Nitpick comments:
In `@app/controllers/katello/api/registry/registry_proxies_controller.rb`:
- Around line 48-50: The HttpResource::HttpError rescue handler (rescue_from
HttpResource::HttpError do |e|) currently logs all misses as errors; change it
to detect a 404 response (e.response_code == 404 or e.response_body includes
"BLOB_UNKNOWN") and log those cases with Rails.logger.info (or downgrade the log
level) instead of Rails.logger.error, preserving the existing body =
e.response_body || e.message assignment and only calling Rails.logger.error for
non-404/ non-BLOB_UNKNOWN errors.
In `@app/lib/actions/candlepin/environment/set_content.rb`:
- Around line 34-47: Replace the if/elsif chain that branches on e.code inside
the add-content rescue block with a case statement (case e.code; when '409' ...
when '404' ... else ... end) to satisfy Style/CaseLikeIf while preserving
existing behavior: on '409' keep the retries increment and raise when (retries
+= 1) == max_retries and refresh output[:add_ids] = content_ids - existing_ids;
on '404' keep the higher retry threshold (raise when (retries += 1) == 1_000),
compute missing_content the same way, log the debug message, and remove it from
output[:add_ids]; otherwise re-raise the exception. Ensure variables and
side-effects (retries, max_retries, output[:add_ids], content_ids, existing_ids,
and the Rails.logger.debug call) remain unchanged.
In `@test/lib/http_resource_test.rb`:
- Around line 30-40: Add a new unit test in test/lib/http_resource_test.rb that
calls TestHttpResource.query_string with parameters containing reserved
characters (e.g., values with spaces, ampersands, and equals like "a b", "c&d",
"e=f") to assert they are percent-encoded in the serialized query string; create
a method such as test_query_string_reserved_characters that passes either a Hash
or array-of-tuples matching the existing ordering expectations and asserts the
expected encoded output (e.g., space -> %20, & -> %26, = -> %3D) to catch
regression in TestHttpResource.query_string encoding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4161b5b-ef78-4794-9ed0-2c091b557af6
📥 Commits
Reviewing files that changed from the base of the PR and between c0e96be and 35dc4fdea5ae2931d82183a9a6901923960acfe4.
📒 Files selected for processing (44)
app/controllers/katello/api/registry/registry_proxies_controller.rbapp/controllers/katello/api/rhsm/candlepin_dynflow_proxy_controller.rbapp/controllers/katello/api/rhsm/candlepin_proxies_controller.rbapp/lib/actions/candlepin/consumer/clean_backend_objects.rbapp/lib/actions/candlepin/environment/add_content_to_environment.rbapp/lib/actions/candlepin/environment/set_content.rbapp/lib/actions/katello/organization/manifest_delete.rbapp/lib/actions/katello/organization/manifest_import.rbapp/lib/actions/katello/organization/manifest_refresh.rbapp/lib/actions/middleware/propagate_candlepin_errors.rbapp/lib/katello/api/v2/error_handling.rbapp/lib/katello/concerns/oauth_request_signing.rbapp/lib/katello/http_resource.rbapp/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/activation_key.rbapp/lib/katello/resources/candlepin/c_p_user.rbapp/lib/katello/resources/candlepin/consumer.rbapp/lib/katello/resources/candlepin/content.rbapp/lib/katello/resources/candlepin/environment.rbapp/lib/katello/resources/candlepin/job.rbapp/lib/katello/resources/candlepin/owner.rbapp/lib/katello/resources/candlepin/pool.rbapp/lib/katello/resources/candlepin/product.rbapp/lib/katello/resources/candlepin/proxy.rbapp/lib/katello/resources/candlepin/subscription.rbapp/lib/katello/resources/candlepin/upstream_consumer.rbapp/lib/katello/resources/candlepin/upstream_entitlement.rbapp/lib/katello/resources/candlepin/upstream_pool.rbapp/lib/katello/util/candlepin_repository_checker.rbapp/models/katello/glue/candlepin/environment.rbapp/models/katello/glue/candlepin/owner.rbapp/models/katello/upstream_pool.rbapp/services/katello/product_content_importer.rbapp/services/katello/registration_manager.rbkatello.gemspectest/controllers/api/rhsm/candlepin_proxies_controller_test.rbtest/lib/http_resource_test.rbtest/lib/resources/candlepin_test.rbtest/lib/tasks/upgrades/4.21/cleanup_python_publications_test.rbtest/models/upstream_pool_test.rbtest/scenarios/scenario_test.rbtest/services/katello/registration_manager_test.rbtest/services/katello/ui_notifications/subscriptions/manifest_expired_warning_test.rbtest/support/scenario_support.rb
Comment on lines
+20
to
+26
| rescue HttpResource::HttpError => e | ||
| display_message = e.message | ||
| if e.response_body.present? | ||
| parsed = JSON.parse(e.response_body) rescue {} | ||
| display_message = parsed['displayMessage'] || display_message | ||
| end | ||
| raise ::Katello::Errors::CandlepinError, display_message |
There was a problem hiding this comment.
Preserve upstream-vs-local error typing here.
This now rewrites every HttpResource::HttpError into Katello::Errors::CandlepinError, so upstream failures lose their specialized class and any downstream rescue logic for UpstreamCandlepinError will stop matching. Please keep the service-specific mapping when re-raising instead of collapsing everything into the local Candlepin type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/actions/middleware/propagate_candlepin_errors.rb` around lines 20 -
26, The rescue currently converts every HttpResource::HttpError into
Katello::Errors::CandlepinError, losing upstream-specific types; change the
re-raise to preserve upstream error classes: after extracting display_message
(using JSON.parse of e.response_body as done), check if
e.is_a?(Katello::Errors::UpstreamCandlepinError) (or whatever upstream subclass
lives under Katello::Errors) and if so re-raise an instance of e.class with the
display_message (e.g. raise e.class, display_message, e.backtrace) so downstream
rescues still match, otherwise raise ::Katello::Errors::CandlepinError with
display_message as today.
Comment on lines
+47
to
49
| @mock_conn.expects(:send).with(:get, '/path').yields(stub_request).returns(@mock_response) | ||
| TestHttpResource.get('/path', headers: { 'headerOne' => 'headerOneValue' }) | ||
| end |
There was a problem hiding this comment.
Assert request mutations, not just send invocation.
Line [113] makes stub_request permissive (headers/body=), so Lines [47], [62], [67], and [72] can pass even if header/body assignment regresses.
Proposed tightening
def test_get
- `@mock_conn.expects`(:send).with(:get, '/path').yields(stub_request).returns(`@mock_response`)
+ request = stub_request
+ `@mock_conn.expects`(:send).with(:get, '/path').yields(request).returns(`@mock_response`)
TestHttpResource.get('/path', headers: { 'headerOne' => 'headerOneValue' })
+ assert_equal({ 'headerOne' => 'headerOneValue' }, request.headers)
end
def test_put
- `@mock_conn.expects`(:send).with(:put, '/path').yields(stub_request).returns(`@mock_response`)
+ request = stub_request(expected_body: { payloadKey: 'payloadValue' })
+ `@mock_conn.expects`(:send).with(:put, '/path').yields(request).returns(`@mock_response`)
TestHttpResource.put('/path', { payloadKey: 'payloadValue' }, headers: { 'headerOne' => 'headerOneValue' })
+ assert_equal({ 'headerOne' => 'headerOneValue' }, request.headers)
end
private
-def stub_request
+def stub_request(expected_body: :any)
req = stub
req.stubs(:headers).returns({})
- req.stubs(:body=)
+ expected_body == :any ? req.stubs(:body=) : req.expects(:body=).with(expected_body)
req
endAlso applies to: 62-64, 67-74, 113-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/lib/http_resource_test.rb` around lines 47 - 49, The test currently only
asserts that `@mock_conn.send` was invoked and yields a permissive stub_request,
which lets regressions in header/body assignment slip by; update the tests
(e.g., the case using `@mock_conn.expects`(:send).with(:get,
'/path').yields(stub_request).returns(`@mock_response`) and the other similar
cases) to set stricter expectations on the yielded stub_request object
itself—expect calls like
stub_request.expects(:headers=).with(hash_including('headerOne' =>
'headerOneValue')) and/or stub_request.expects(:body=).with(expected_body) (or
use assert_received/assert_called equivalents) before invoking
TestHttpResource.get to ensure headers/body are actually assigned rather than
only verifying send was called.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/lib/katello/resources/candlepin/activation_key.rb (1)
69-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard empty-body parsing in
update_content_overrides.At Line 69,
result&.body || '{}'still passes''through, which can raiseJSON::ParserErroron empty delete responses. Treat blank body as{}before parsing.Proposed fix
- ::Katello::Util::Data.array_with_indifferent_access(JSON.parse(result&.body || '{}')) + body = result&.body.presence || '{}' + ::Katello::Util::Data.array_with_indifferent_access(JSON.parse(body))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/resources/candlepin/activation_key.rb` at line 69, The JSON.parse call in update_content_overrides uses result&.body || '{}' which still lets an empty string get parsed and raise JSON::ParserError; before calling JSON.parse (the expression currently written as ::Katello::Util::Data.array_with_indifferent_access(JSON.parse(result&.body || '{}'))), normalize the body by converting result&.body to a string and treating blank/whitespace-only bodies as '{}' (e.g. use result&.body.to_s.strip.empty? ? '{}' : result.body) so JSON.parse always receives a valid JSON string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/katello/resources/candlepin/upstream_consumer.rb`:
- Around line 14-15: The two exception signals in upstream_consumer.rb should
use fail instead of raise to satisfy Style/SignalException (semantic) — replace
the initial uses of raise ::Katello::Errors::UpstreamConsumerGone and raise
::Katello::Errors::UpstreamConsumerNotFound (the branches checking
response.status for [401, 410] and 404) with fail so the UpstreamConsumer error
signaling conforms to the lint rule; keep the same exception classes and
conditions (response.status checks) unchanged.
---
Duplicate comments:
In `@app/lib/katello/resources/candlepin/activation_key.rb`:
- Line 69: The JSON.parse call in update_content_overrides uses result&.body ||
'{}' which still lets an empty string get parsed and raise JSON::ParserError;
before calling JSON.parse (the expression currently written as
::Katello::Util::Data.array_with_indifferent_access(JSON.parse(result&.body ||
'{}'))), normalize the body by converting result&.body to a string and treating
blank/whitespace-only bodies as '{}' (e.g. use result&.body.to_s.strip.empty? ?
'{}' : result.body) so JSON.parse always receives a valid JSON string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 762acb98-d805-442b-90b3-e63d26f431d6
📥 Commits
Reviewing files that changed from the base of the PR and between 35dc4fdea5ae2931d82183a9a6901923960acfe4 and 35986c571dfa9db6044701c09e3f74cc9225ce50.
📒 Files selected for processing (6)
app/lib/katello/resources/candlepin/activation_key.rbapp/lib/katello/resources/candlepin/consumer.rbapp/lib/katello/resources/candlepin/owner.rbapp/lib/katello/resources/candlepin/upstream_consumer.rbapp/lib/katello/resources/candlepin/upstream_job.rbtest/lib/resources/candlepin_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/katello/resources/candlepin/consumer.rb
- app/lib/katello/resources/candlepin/owner.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/katello/resources/candlepin.rb`:
- Around line 154-165: The code disables SSL verification when ca_file is nil
which makes the subsequent proxy.cacert-based cert_store (used via
Foreman::Util.add_ca_bundle_to_store and assigned to f.ssl.cert_store)
ineffective; change the logic so that when proxy&.cacert is present you enable
peer verification (set f.ssl.verify = true) and populate f.ssl.cert_store with
the cert_store, falling back to disabling verification only if neither ca_file
nor proxy.cacert are available—update the conditional flow around ca_file,
proxy.cacert, f.ssl.verify and f.ssl.cert_store in the Candlepin Faraday setup
to reflect this.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59ecebca-290e-417c-b926-24498e830a8a
📥 Commits
Reviewing files that changed from the base of the PR and between 35986c571dfa9db6044701c09e3f74cc9225ce50 and 832c215cc74e46365d7af3a0f64a6b571705229b.
📒 Files selected for processing (6)
app/lib/katello/http_resource.rbapp/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/activation_key.rbapp/lib/katello/resources/candlepin/consumer.rbapp/lib/katello/resources/candlepin/upstream_consumer.rbtest/lib/resources/candlepin_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lib/resources/candlepin_test.rb
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/lib/actions/middleware/propagate_candlepin_errors.rb (1)
22-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse upstream HTTP failures into
CandlepinError.
UpstreamPool.getnow routes non-401/410 upstream failures throughprocess_response, so an upstream 5xx reaches this rescue asHttpResource::HttpErrorand gets re-raised here as plain::Katello::Errors::CandlepinError. That drops the upstream/local distinction and can break downstream rescues keyed on upstream failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/actions/middleware/propagate_candlepin_errors.rb` around lines 22 - 28, The rescue currently converts every HttpResource::HttpError into ::Katello::Errors::CandlepinError, losing upstream-vs-local distinction; update the rescue in propagate_candlepin_errors.rb so that if the caught HttpResource::HttpError represents an upstream failure (e.g. response code in the 5xx range or otherwise coming from UpstreamPool/process_response), re-raise the original HttpResource::HttpError (raise e) to preserve its type, otherwise continue to extract display_message from e.response_body and raise ::Katello::Errors::CandlepinError with that message; reference the HttpResource::HttpError rescue block and ::Katello::Errors::CandlepinError and consider using e.response_code or any upstream marker set by UpstreamPool.get/process_response to decide which path to take.app/lib/katello/http_resource.rb (1)
68-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve plain-text error bodies in
HttpError#message.When JSON parsing fails, Line 71 replaces the upstream body with the parser exception text.
PropagateCandlepinErrorsseeds its display message frome.message, so plain-text Candlepin failures will surface as JSON parse noise instead of the actual response body. Preferresp.body.presence || error.to_shere.Suggested fix
- fail HttpError, {:message => error.to_s, :service_code => service_code, :code => status_code, :response_body => resp.body}, caller + fail HttpError, {:message => resp.body.presence || error.to_s, :service_code => service_code, :code => status_code, :response_body => resp.body}, caller🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/http_resource.rb` around lines 68 - 71, In the rescue block that raises HttpError (the rescue => error in app/lib/katello/http_resource.rb), preserve the original response body when building the HttpError message instead of always using the parser exception; change the error payload so :message is resp.body.presence || error.to_s (or resp.body || error.to_s if presence isn't available), keeping the rest of the HttpError arguments the same so PropagateCandlepinErrors can surface plain-text Candlepin responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/lib/actions/middleware/propagate_candlepin_errors.rb`:
- Around line 22-28: The rescue currently converts every HttpResource::HttpError
into ::Katello::Errors::CandlepinError, losing upstream-vs-local distinction;
update the rescue in propagate_candlepin_errors.rb so that if the caught
HttpResource::HttpError represents an upstream failure (e.g. response code in
the 5xx range or otherwise coming from UpstreamPool/process_response), re-raise
the original HttpResource::HttpError (raise e) to preserve its type, otherwise
continue to extract display_message from e.response_body and raise
::Katello::Errors::CandlepinError with that message; reference the
HttpResource::HttpError rescue block and ::Katello::Errors::CandlepinError and
consider using e.response_code or any upstream marker set by
UpstreamPool.get/process_response to decide which path to take.
In `@app/lib/katello/http_resource.rb`:
- Around line 68-71: In the rescue block that raises HttpError (the rescue =>
error in app/lib/katello/http_resource.rb), preserve the original response body
when building the HttpError message instead of always using the parser
exception; change the error payload so :message is resp.body.presence ||
error.to_s (or resp.body || error.to_s if presence isn't available), keeping the
rest of the HttpError arguments the same so PropagateCandlepinErrors can surface
plain-text Candlepin responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 861316c2-49f2-4c75-aadd-a353fd4e065c
📥 Commits
Reviewing files that changed from the base of the PR and between 832c215cc74e46365d7af3a0f64a6b571705229b and ac34a649a6d3c63217a4211624fea02096623ed9.
📒 Files selected for processing (7)
app/controllers/katello/api/registry/registry_proxies_controller.rbapp/lib/actions/middleware/propagate_candlepin_errors.rbapp/lib/katello/http_resource.rbapp/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/upstream_consumer.rbapp/lib/katello/resources/candlepin/upstream_pool.rbtest/support/scenario_support.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/controllers/katello/api/registry/registry_proxies_controller.rb
- test/support/scenario_support.rb
- app/lib/katello/resources/candlepin/upstream_consumer.rb
- app/lib/katello/resources/candlepin.rb
pablomh
force-pushed
the
faraday-persistent-connections
branch
from
ac34a64 to
f3669c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/katello/concerns/oauth_request_signing.rb`:
- Line 31: Remove the trailing comma from the hash literal that includes the
:ca_file => self.ssl_ca_file entry in the OAuthRequestSigning concern; locate
the options/hash construction (references: :ca_file key and ssl_ca_file method)
and delete the final comma so the argument list/hard-coded hash is valid for
RuboCop.
- Around line 17-19: The guard in sign_request currently only rejects nil/false
and allows empty strings; update the check in sign_request to reject blank
credentials by verifying consumer_key and consumer_secret are not
nil/empty/whitespace (e.g. use consumer_key.to_s.strip.empty? /
consumer_secret.to_s.strip.empty? or consumer_key.blank? if ActiveSupport is
available) and raise the same failure message if either is blank before calling
build_oauth_header so misconfiguration surfaces immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80bf860c-e16d-4c92-9310-5a75a8b37033
📥 Commits
Reviewing files that changed from the base of the PR and between ac34a649a6d3c63217a4211624fea02096623ed9 and f3669c459387fb24a852ff11d37402a54d60f714.
📒 Files selected for processing (11)
app/controllers/katello/api/registry/registry_proxies_controller.rbapp/lib/actions/candlepin/environment/set_content.rbapp/lib/actions/middleware/propagate_candlepin_errors.rbapp/lib/katello/api/v2/error_handling.rbapp/lib/katello/concerns/oauth_request_signing.rbapp/lib/katello/http_resource.rbapp/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/upstream_consumer.rbapp/lib/katello/resources/candlepin/upstream_pool.rbapp/services/katello/registration_manager.rbtest/support/scenario_support.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- app/lib/katello/resources/candlepin/upstream_pool.rb
- app/services/katello/registration_manager.rb
- app/lib/actions/middleware/propagate_candlepin_errors.rb
- app/lib/katello/api/v2/error_handling.rb
- app/lib/katello/resources/candlepin/upstream_consumer.rb
- app/lib/actions/candlepin/environment/set_content.rb
- app/lib/katello/resources/candlepin.rb
- app/lib/katello/http_resource.rb
pablomh
force-pushed
the
faraday-persistent-connections
branch
from
bcc28f1 to
f899012
Compare
pablomh
force-pushed
the
faraday-persistent-connections
branch
2 times, most recently
from
c5bcdbd to
26bd7d5
Compare
Member
Don't forget https://github.com/theforeman/foreman_google that also pins to Faraday < 2. |
ekohl
reviewed
May 20, 2026
| Rails.logger.error(pp_exception(e, with_backtrace: false)) | ||
| end | ||
| body = e.response_body.presence || e.message | ||
| if request_from_katello_cli? |
Member
There was a problem hiding this comment.
I'm surprised by this mechanism. I know it was already there, but Rails normally uses respond_to. I think this would be the Rails native mechanism, assuming the CLI sets Accept: application/json as a header:
respond_to do |format|
format.json { render json: { errors: [body] }, status: e.code }
format.any { render plain: body, status: e.code }
end
Contributor
Author
There was a problem hiding this comment.
I checked the current Hammer stack and it does reliably send Accept: application/json;version=... via apipie-bindings, so respond_to should work for the maintained CLI path. You're also right that request_from_katello_cli? appears to be legacy behavior tied to the old Python-based Katello CLI rather than modern Hammer. My only hesitation is calling it fully dead code without scoping that claim — the registry endpoints are more likely hit by container tooling (podman, skopeo) and scripts than by Hammer itself. If we're comfortable aligning this with maintained clients rather than preserving the old User-Agent behavior, I think respond_to is reasonable — but I'd do it as a follow-up that updates all the sibling controllers consistently rather than just this one handler.
Member
There was a problem hiding this comment.
Agreed it belongs in a separate PR.
pablomh
force-pushed
the
faraday-persistent-connections
branch
from
37c9372 to
421fdc2
Compare
Replace the unmaintained RestClient gem with Faraday 2.x and the net_http_persistent adapter to enable persistent TCP+TLS connection reuse between Puma and Candlepin, eliminating per-request connection setup overhead during concurrent host registration. Key changes: - Replace RestClient::Resource with Faraday::Connection, memoized at the class level for connection pooling across Puma threads - Extract OAuth 1.0a request signing into OauthRequestSigning concern, making the auth mechanism a pluggable strategy (preparation for CANDLEPIN-222 mTLS transition) - Add sign_request hook on HttpResource as extension point - Memoize OAuth::Consumer object to avoid per-request allocation - Fix Proxy class to sign requests via concern - Fix CandlepinProxiesController to use Faraday response API - Fix DELETE-with-body paths to include OAuth signing - Fix UpstreamEntitlement bracket notation (RestClient-era API) - Add defensive stringify_headers helper for Net::HTTP compatibility - Update gemspec: faraday >= 2.0, faraday-net_http_persistent, faraday-multipart Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add process: false option to issue_request for pass-through callers that need raw Faraday::Response without error raising - Rewrite Proxy to delegate to CandlepinResource.issue_request instead of duplicating connection, signing, and header logic - Route UpstreamPool.get through issue_request for consistent logging and connection handling - Memoize UpstreamCandlepinResource.faraday_connection for connection reuse on upstream manifest operations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- rescue_from_exception_with_response now uses the actual HTTP status code from HttpError instead of hardcoding :bad_request (resolves long-standing TODO) - upstream_owner_id routes through issue_request instead of raw conn.get with rescue Faraday::Error - Add comment on resource vs faraday_connection lifecycle in UpstreamCandlepinResource Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generated get/delete now accept (path, headers:, params:). Generated post/put/patch now accept (path, payload, headers:, params:). Eliminates fragile positional arg parsing and makes params: available to all callers without dropping to issue_request. All 41 call sites and super calls updated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- rescue_from_exception_with_response falls back to :bad_request when code.to_i yields 0 (empty or missing code) - Add comment on Owner#import explaining why multipart bypasses issue_request Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- UpstreamConsumer#ping uses issue_request instead of resource + conn.head - Consumer and ActivationKey DELETE-with-body use issue_request instead of raw conn.delete with manual signing and headers - UpstreamEntitlement#update uses issue_request instead of resource + conn.put Only Owner#import (multipart, documented) and three UpstreamConsumer methods (custom URL/certs per call) remain outside issue_request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add connection: parameter to issue_request so callers with custom Faraday connections (different URL/certs) still go through the unified request pipeline (logging, signing, headers, error handling). All Candlepin HTTP traffic now flows through issue_request. Only Owner#import (multipart with separate Faraday stack) remains outside. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When connection: is provided, the path alone is insufficient for debugging since the host varies per call. Now logs the full URL (url_prefix + path) for custom connections, plain path otherwise. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- UpstreamConsumer#ping: check response.status directly instead of relying on HttpError rescue (process: false never raises HttpError) - OAuth signing URL now derived from conn.url_prefix when connection: is provided, ensuring correct signing for any future OAuth-backed custom connection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Test issue_request with custom connection: parameter - Test process: false returns raw Faraday::Response - Test UpstreamConsumer#ping maps 401/410 to UpstreamConsumerGone and 404 to UpstreamConsumerNotFound - Verify no positional header calls in engines/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use logger.debug { ... } blocks in issue_request and process_response
so serialization (to_json, filter_sensitive_data) is skipped when
debug logging is disabled
- Consolidate duplicate url/sign_url computation into single full_url
- Memoize upstream_owner_id to avoid repeated GET on every upstream
path resolution (UpstreamPool.path, UpstreamOwner.path)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Memoized faraday_connection and upstream_owner_id can become stale after manifest refresh changes the upstream identity cert. Callers can invoke reset_connection! to clear cached state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clears memoized faraday_connection and upstream_owner_id after manifest operations that may change the upstream identity cert, preventing stale credentials on subsequent upstream pool listings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both faraday_connection and upstream_owner_id are now keyed by Organization.current.id, preventing cross-org data leaks in multi-threaded Puma serving multiple organizations concurrently. reset_connection! clears only the current org's cached entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dynflow workers may not have Organization.current set during manifest finalize, so per-org deletion would miss the target. Full clear is safe since manifest operations are rare and rebuild cost is trivial. Also prevents unbounded hash growth in long-running processes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion - Owner.destroy_imports, Owner.imports: parse response.body not response - Consumer.async_hypervisors: parse response.body - Consumer.update_content_overrides: parse result.body, early return on empty - ActivationKey.update_content_overrides: parse result&.body, early return on empty - UpstreamConsumer.get: fix ?/& separator for include_only with empty params - UpstreamJob.get: parse response.body from start_upstream_export - Consumer.get: safe query string construction matching UpstreamConsumer pattern - Owner.import: add open_timeout for consistency with other Faraday connections - Add tests for Consumer.get URL composition and empty content-override edge cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…des_for - build_path: safe URL composition for base path + params + includes + pagination, eliminating manual ?/& assembly in Consumer.get and UpstreamConsumer.get - parse_json: shared JSON response parsing with optional array mode, available for gradual adoption across resource classes - update_content_overrides_for: deduplicate identical logic between Consumer and ActivationKey, preventing fix drift - Dup params before mutating with .delete(:include_only) to avoid leaking side effects to callers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Style/MutableConstant: freeze TOTAL_COUNT_HEADER - Style/SignalException: use fail instead of raise - Style/CaseLikeIf: convert if-elsif to case-when in http_resource - Lint/UnusedMethodArgument: prefix unused id with underscore - Minitest/AssertEmptyLiteral: use assert_empty instead of assert_equal [] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add trailing comma after last item in multiline keyword args in update_content_overrides_for issue_request call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- registry_proxies_controller: use .presence for empty body fallback - propagate_candlepin_errors: re-raise CandlepinError subclasses to preserve upstream error typing - http_resource: keep non-JSON error responses as HttpError instead of downgrading to NetworkException - upstream_owner_id: remove process: false so HTTP errors are handled normally instead of silently returning nil - upstream_pool: guard non-2xx responses, map 401 to UpstreamConsumerGone, delegate other errors to process_response - scenario_support: normalize code comparison with .to_s - upstream resource: set ssl.verify based on whether trust anchors (ca_file or proxy cacert) are available, not unconditionally false Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align exception signaling and case-style handling with the project RuboCop rules so CI passes without changing the Faraday migration behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Clarify the HTTP-versus-network failure boundary and harden the shared Candlepin helpers so the Faraday migration preserves upstream messages, custom ports, and cleanup safety. Co-authored-by: Cursor <cursoragent@cursor.com>
Lock in the new transport, upstream URL, empty-body parsing, and cleanup restoration behavior so the Faraday follow-up remains reviewable and stable. Co-authored-by: Cursor <cursoragent@cursor.com>
- UpstreamPoolTest: stub response with body: and status: instead of to_str: (Faraday response API, not RestClient) - CoreTest: replace test_with_excon with test_with_net_http_persistent (excon adapter not available in Faraday 2.x) - ManifestExpiredWarningTest: stub cdn_inaccessible? directly instead of raising HttpError from Net::HTTP (CDN uses RestClient, not Faraday) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Pulp3 publications API path uses PulpcoreClient (which depends on RestClient), not our Faraday-based HttpResource. The production code at cleanup_python_publications.rake:147 rescues RestClient::NotFound, so the test must raise that same exception type. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update RHSM routing expectations and permission coverage so the new compliance endpoints match their dedicated actions in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
- Revert routing test and access_permissions_test changes that belong to PR Katello#11731, not this branch (compliance routes unchanged here) - Fix UpstreamPoolTest header stub: use string key 'x-total-count' instead of symbol x_total_count (Faraday doesn't auto-normalize) - Fix trailing slash in ConsumerResource/OwnerResource path when id is nil: /consumers/ -> /consumers (Faraday doesn't strip it) - Add :net_http_persistent to pulp3_ssl_configuration case statement alongside :net_http (same SSL cert types) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use the same pessimistic Faraday 2.x dependency constraint as the matching smart-proxy transport change for consistency across the related migrations. Co-authored-by: Cursor <cursoragent@cursor.com>
pablomh
force-pushed
the
faraday-persistent-connections
branch
from
3d91add to
7de46e8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Replace the unmaintained RestClient gem with Faraday 2.x and the net_http_persistent adapter for the Candlepin HTTP layer, enabling persistent TCP+TLS connection reuse between Puma and Candlepin. This eliminates per-request connection setup overhead during concurrent host registration.
Prerequisite: theforeman/foreman_azure_rm#209 must merge first — the retired Azure SDK pins faraday < 2 via ms_rest, which blocks this upgrade.
Note: rest-client remains as a dependency — it is still used by CDN resource code (cdn.rb). Only the Candlepin HTTP layer is migrated to Faraday.
Key changes:
Considerations taken when implementing this change?
Gemspec changes:
Added:
Retained (still used by CDN):
Packaging note: RPM packages need to add rubygem-faraday, rubygem-faraday-net_http_persistent, rubygem-faraday-multipart, and rubygem-net-http-persistent. Do NOT remove rubygem-rest-client — it is still required by CDN resource code.
What are the testing steps for this pull request?
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores