Add yum_repos to slice #1053
Open
Add yum_repos to slice #1053
Conversation
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
Reviewer's GuideThis PR conditionally adds a yum_repos node to system_profile in IoP mode by extending the slice generator with new reporting methods, adds a corresponding unit test, refactors Katello factories for valid UUIDs, and updates related tests to use the content_facet factory. Sequence diagram for conditional yum_repos node addition in IoP modesequenceDiagram
participant Slice
participant Host
participant ForemanRhCloud
participant Stream
participant Repo
participant Content
participant ContentSource
Slice->>Host: access content_facet.bound_repositories
alt bound_repositories present
Slice->>ForemanRhCloud: with_local_advisor_engine?
alt IoP mode
Slice->>Stream: array_field('yum_repos')
loop for each repo
Slice->>ContentSource: load_balancer_pulp_content_url
Slice->>Repo: generate_repo_path(content_url)
Slice->>Stream: object()
Slice->>Stream: simple_field('name', repo.content.name)
Slice->>Stream: simple_field('id', repo.content.label)
Slice->>Stream: simple_field('base_url', ...)
Slice->>Stream: simple_field('enabled', true)
Slice->>Stream: simple_field('gpgcheck', true, :last)
opt not last repo
Slice->>Stream: comma()
end
end
end
end
Class diagram for updated Slice generator with yum_repos reportingclassDiagram
class Slice {
+report_system_profile(host, host_ips_cache)
+report_yum_repos(host)
+report_yum_repo(url_prefix, repo)
...
}
Slice --> Stream : uses
class Stream {
+array_field(name)
+object()
+simple_field(name, value, last?)
+comma()
...
}
class Host {
+content_facet
+content_source
...
}
Host --> ContentFacet : has
Host --> ContentSource : has
class ContentFacet {
+bound_repositories
...
}
class ContentSource {
+load_balancer_pulp_content_url
...
}
class Repo {
+content
+generate_repo_path(content_url)
...
}
class Content {
+name
+label
+content_url
...
}
ContentFacet --> Repo : has many
Repo --> Content : has
Slice ..> ForemanRhCloud : calls with_local_advisor_engine?
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ShimShtein - I've reviewed your changes - here's some feedback:
- Consider splitting the large Katello factory clean‐up into a separate PR to keep the yum_repos feature changes focused and easier to review.
- Add a guard for missing content_source in report_yum_repos to prevent a potential NoMethodError when bound_repositories exist but content_source is nil.
- You could cache the load_balancer_pulp_content_url outside the loop in report_yum_repos to avoid calling it repeatedly for each repository.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider splitting the large Katello factory clean‐up into a separate PR to keep the yum_repos feature changes focused and easier to review.
- Add a guard for missing content_source in report_yum_repos to prevent a potential NoMethodError when bound_repositories exist but content_source is nil.
- You could cache the load_balancer_pulp_content_url outside the loop in report_yum_repos to avoid calling it repeatedly for each repository.
## Individual Comments
### Comment 1
<location> `lib/foreman_inventory_upload/generators/slice.rb:260` </location>
<code_context>
+
+ @stream.array_field('yum_repos') do
+ host.content_facet.bound_repositories.each_with_index do |repo, index|
+ report_yum_repo(host.content_source.load_balancer_pulp_content_url, repo)
+ @stream.comma unless index == host.content_facet.bound_repositories.count - 1
+ end
</code_context>
<issue_to_address>
Potential for nil host.content_source could cause errors.
Add a check to handle cases where `host.content_source` is nil before calling `load_balancer_pulp_content_url`. This will prevent potential runtime errors when processing hosts without a content source.
</issue_to_address>
### Comment 2
<location> `lib/foreman_inventory_upload/generators/slice.rb:270` </location>
<code_context>
+ @stream.object do
+ @stream.simple_field('name', repo.content.name)
+ @stream.simple_field('id', repo.content.label)
+ @stream.simple_field('base_url', "#{url_prefix}/#{repo.generate_repo_path(repo.content.content_url)}")
+ @stream.simple_field('enabled', true)
+ @stream.simple_field('gpgcheck', true, :last)
</code_context>
<issue_to_address>
String interpolation for base_url may result in double slashes.
Normalize `url_prefix` and the path from `generate_repo_path` to prevent double slashes in the final URL.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
@stream.object do
@stream.simple_field('name', repo.content.name)
@stream.simple_field('id', repo.content.label)
@stream.simple_field('base_url', "#{url_prefix}/#{repo.generate_repo_path(repo.content.content_url)}")
@stream.simple_field('enabled', true)
@stream.simple_field('gpgcheck', true, :last)
end
=======
@stream.object do
@stream.simple_field('name', repo.content.name)
@stream.simple_field('id', repo.content.label)
normalized_url_prefix = url_prefix.to_s.sub(%r{/*\z}, '')
repo_path = repo.generate_repo_path(repo.content.content_url).to_s.sub(%r{\A/+}, '')
@stream.simple_field('base_url', "#{normalized_url_prefix}/#{repo_path}")
@stream.simple_field('enabled', true)
@stream.simple_field('gpgcheck', true, :last)
end
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `test/unit/slice_generator_test.rb:1001` </location>
<code_context>
+ test 'reports yum repos' do
</code_context>
<issue_to_address>
Missing negative and edge case tests for yum_repos reporting
Please add tests for cases where ForemanRhCloud.with_local_advisor_engine? returns false, the host has no bound repositories, and bound_repositories is empty, to ensure correct behavior in these edge scenarios.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| @stream.array_field('yum_repos') do | ||
| host.content_facet.bound_repositories.each_with_index do |repo, index| | ||
| report_yum_repo(host.content_source.load_balancer_pulp_content_url, repo) |
There was a problem hiding this comment.
issue (bug_risk): Potential for nil host.content_source could cause errors.
Add a check to handle cases where host.content_source is nil before calling load_balancer_pulp_content_url. This will prevent potential runtime errors when processing hosts without a content source.
Comment on lines
+267
to
+273
| @stream.object do | ||
| @stream.simple_field('name', repo.content.name) | ||
| @stream.simple_field('id', repo.content.label) | ||
| @stream.simple_field('base_url', "#{url_prefix}/#{repo.generate_repo_path(repo.content.content_url)}") | ||
| @stream.simple_field('enabled', true) | ||
| @stream.simple_field('gpgcheck', true, :last) | ||
| end |
There was a problem hiding this comment.
suggestion (bug_risk): String interpolation for base_url may result in double slashes.
Normalize url_prefix and the path from generate_repo_path to prevent double slashes in the final URL.
Suggested change
| @stream.object do | |
| @stream.simple_field('name', repo.content.name) | |
| @stream.simple_field('id', repo.content.label) | |
| @stream.simple_field('base_url', "#{url_prefix}/#{repo.generate_repo_path(repo.content.content_url)}") | |
| @stream.simple_field('enabled', true) | |
| @stream.simple_field('gpgcheck', true, :last) | |
| end | |
| @stream.object do | |
| @stream.simple_field('name', repo.content.name) | |
| @stream.simple_field('id', repo.content.label) | |
| normalized_url_prefix = url_prefix.to_s.sub(%r{/*\z}, '') | |
| repo_path = repo.generate_repo_path(repo.content.content_url).to_s.sub(%r{\A/+}, '') | |
| @stream.simple_field('base_url', "#{normalized_url_prefix}/#{repo_path}") | |
| @stream.simple_field('enabled', true) | |
| @stream.simple_field('gpgcheck', true, :last) | |
| end |
Comment on lines
+1001
to
+1010
| test 'reports yum repos' do | ||
| ForemanRhCloud.stubs(:with_local_advisor_engine?).returns(true) | ||
| FactoryBot.create(:katello_content, cp_content_id: '1', organization: @host.organization, name: 'Test Content', label: 'test-content') | ||
| repo = FactoryBot.build( | ||
| :katello_repository, | ||
| :fedora_17_x86_64_dev, | ||
| :with_content_view, | ||
| product: FactoryBot.create(:katello_product, :redhat, :with_provider, organization: @host.organization) | ||
| ) | ||
| # force-save the root repo to avoid validation errors |
There was a problem hiding this comment.
suggestion (testing): Missing negative and edge case tests for yum_repos reporting
Please add tests for cases where ForemanRhCloud.with_local_advisor_engine? returns false, the host has no bound repositories, and bound_repositories is empty, to ensure correct behavior in these edge scenarios.
There was a problem hiding this comment.
@ShimShtein did you want to add that kind of test^
|
Starting to look at this one |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
This PR adds yum_repos node to the report, only in IoP case.
Considerations taken when implementing this change?
I did not want to disrupt the hosted use case, so added the node only if we are in IoP mode
What are the testing steps for this pull request?
Generate and upload an rh_cloud report and observe the yum_repos node under system_profile.
Summary by Sourcery
Add yum_repos reporting to the system_profile slice generator in IoP mode and update tests and factories accordingly
New Features:
Enhancements:
Tests: