Fixes #38862 - Switch to subscription facet for uuids#11569
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/models/concerns/smart_proxy_extensions_test.rb:438` </location>
<code_context>
SmartProxy.joins(:smart_proxy_alternate_content_sources).where('katello_smart_proxy_alternate_content_sources.smart_proxy_id' => self.id)
end
+ def subscription_facets
</code_context>
<issue_to_address>
**suggestion (testing):** Tests do not cover error handling/logging for API failures in sync_container_gateway.
Please add tests that simulate exceptions in these API calls and verify that error logging occurs as expected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -436,6 +436,108 @@ def test_sync_container_gateway | |||
| capsule_content.smart_proxy.expects(:container_gateway_users).returns(::User.where(login: 'secret_admin')) | |||
| capsule_content.smart_proxy.sync_container_gateway | |||
| end | |||
Contributor
There was a problem hiding this comment.
suggestion (testing): Tests do not cover error handling/logging for API failures in sync_container_gateway.
Please add tests that simulate exceptions in these API calls and verify that error logging occurs as expected.
jeremylenz
reviewed
Nov 17, 2025
sjha4
force-pushed
the
sp_sync_nil_facet_uuid
branch
from
35fb928 to
a59c3a3
Compare
ianballou
reviewed
Nov 19, 2025
| host_repo_map = { hosts: [] } | ||
| content_facets.each do |facet| | ||
| subscription_facets.each do |facet| | ||
| next if facet.uuid.blank? | ||
| repositories = ::Katello::Repository.readable_docker_catalog(facet.host) | ||
| host_repo_map[:hosts] << { facet.uuid => build_repo_list(repositories) } | ||
| end | ||
| ProxyAPI::ContainerGateway.new(url: self.url).host_repository_mapping(host_repo_map) | ||
| end | ||
|
|
||
| def update_host_repositories(host) |
Member
There was a problem hiding this comment.
Funnily enough I don't think this method is called anywhere. Can it be removed?
ianballou
reviewed
Nov 19, 2025
Member
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
This seems to be working well, I made a content facet UUID nil and I was still able to run sync_container_gateway. Tested quickly with a cert-using host.
Comment on lines
+271
to
+272
| facets ||= subscription_facets | ||
| hosts = facets.select { |facet| facet.uuid.present? }.map do |facet| |
Member
There was a problem hiding this comment.
Suggested change
| facets ||= subscription_facets | |
| hosts = facets.select { |facet| facet.uuid.present? }.map do |facet| | |
| facets ||= subscription_facets.where.not(uuid: nil) | |
| hosts = facets.map do |facet| |
Gets rid of a block going to a block.
Member
Author
There was a problem hiding this comment.
Updated to move this to subscription_facets function directly..So both callers can rely on the filter being there.
ianballou
reviewed
Nov 19, 2025
sjha4
force-pushed
the
sp_sync_nil_facet_uuid
branch
from
a59c3a3 to
798fd9d
Compare
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
zjhuntin
pushed a commit
to zjhuntin/katello
that referenced
this pull request
Mar 13, 2026
zjhuntin
pushed a commit
that referenced
this pull request
Mar 16, 2026
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
3 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?
Considerations taken when implementing this change?
subscription_facets is a method to grab the sub facets instead of a relation on smart_proxy to avoid extra relations and dependencies. Something a test failure raised as a flag with claude.
What are the testing steps for this pull request?
Register a host via a smart proxy.
Do something like:
cf = smart_proxy.content_facets.first
cf.uuid = nil
cf.save!
Try a smart proxy sync. You'll see an error :
With this change, you should be able to correctly sync even with nil content_facet uuid.
For completeness, you can also set subscription facet uuid to nil and try a sync. The sync will still succeed and nil UUID will be correctly ignored.
Summary by Sourcery
Use subscription_facet UUID as the source of truth for container gateway host and repository updates, gracefully skip missing UUIDs, and add error handling and logging.
Bug Fixes:
Enhancements:
Tests: