Fixes #38905 - Add ACS for Deb Repositories#11567
Merged
m-bucher merged 1 commit intoKatello:masterfrom Apr 23, 2026
Merged
Conversation
hstct
force-pushed
the
add-debian-acs
branch
2 times, most recently
from
5397ab9 to
2cb150a
Compare
sbernhard
force-pushed
the
add-debian-acs
branch
2 times, most recently
from
c4d381d to
feb2440
Compare
11 tasks
hstct
force-pushed
the
add-debian-acs
branch
from
feb2440 to
d684c39
Compare
Contributor
|
Just adding some ToDo reminders for ourselves:
|
Contributor
Author
|
once pulp_deb |
quba42
force-pushed
the
add-debian-acs
branch
from
d684c39 to
1e92c73
Compare
hstct
force-pushed
the
add-debian-acs
branch
2 times, most recently
from
0a1ea5c to
99e2c57
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @hstct, your pull request is larger than the review limit of 150000 diff characters
hstct
force-pushed
the
add-debian-acs
branch
3 times, most recently
from
c33ef52 to
d81e9e5
Compare
quba42
reviewed
Jan 28, 2026
aidenfine
reviewed
Mar 26, 2026
📝 WalkthroughWalkthroughAdds Debian (DEB) alternate content source support across backend and frontend: new DEB fields, model predicates/validations, controller pre-validation, Pulp3 service/API mappings, DB migration, frontend form/state/helpers, orchestration logic, VCR fixtures, and expanded tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/User
participant API as API Controller
participant Model as AlternateContentSource Model
participant Service as Pulp3 Service
participant Pulp as Pulp3 API
Client->>API: POST /api/v2/alternate_content_sources (deb params)
API->>API: permit params, run validate_acs_params!
API->>Model: build ACS from params (excluding proxy/product ids)
Model->>Model: run deb? validations (deb_constraints, deb_fields_xor_subpaths)
alt validation fails
Model-->>API: errors (message, errors, full_messages)
API-->>Client: 422 Unprocessable Entity
else validation passes
API->>Service: build remote_options(acs)
Service->>Service: join_space_required!(distributions)
Service->>Service: join_space_or_nil(components/architectures)
Service->>Pulp: POST /pulp/api/v3/remotes/deb/apt/ (create remote)
Pulp-->>Service: remote href
Service->>Pulp: POST /pulp/api/v3/acs/deb/deb/ (create ACS)
Pulp-->>Service: ACS href / task
Service-->>API: task/href
API-->>Client: 201 Created (ACS resource / task)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/katello/pulp3/smart_proxy_mirror_repository.rb (1)
140-167:⚠️ Potential issue | 🟡 MinorMissing return statement for
tasksarray.The
delete_orphan_alternate_content_sourcesmethod populates thetasksarray but doesn't return it. Other similar methods in this file (e.g.,delete_orphan_repositories,delete_orphan_distributions,delete_orphan_remotes) return their tasks array. This may cause issues if the caller expects task references for monitoring.🐛 Proposed fix
if RepositoryTypeManager.enabled_repository_types['deb'] deb_acs_api = ::Katello::Pulp3::Repository.api(smart_proxy, 'deb').alternate_content_source_api orphan_deb_acs_hrefs = deb_acs_api.list.results.map(&:pulp_href) - known_acs_hrefs orphan_deb_acs_hrefs.each do |orphan_deb_acs_href| tasks << deb_acs_api.delete(orphan_deb_acs_href) end end + tasks end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/katello/pulp3/smart_proxy_mirror_repository.rb` around lines 140 - 167, The method delete_orphan_alternate_content_sources builds a local tasks array but never returns it; update delete_orphan_alternate_content_sources to return the tasks array (same behavior as delete_orphan_repositories / delete_orphan_distributions / delete_orphan_remotes) so callers can receive the created task references from the file_acs_api/yum_acs_api/deb_acs_api delete calls.
🧹 Nitpick comments (6)
webpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.js (2)
102-105: TheuseSelectormock placement may be fragile.Setting up the
useSelectormock mid-test after some interactions have already occurred could lead to inconsistent behavior. Consider moving the mock setup before rendering or using a more predictable pattern for providing test data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.js` around lines 102 - 105, The test sets up the reactRedux.useSelector mock (useSelectorMock) after interactions, which is fragile; move the jest.spyOn(reactRedux, 'useSelector') and its mockReturnValueOnce(smartProxyResult).mockReturnValue(contentCredentialResult.results) setup to run before rendering or user interactions (e.g., at the start of the test or in a beforeEach) so useSelector returns predictable values during render and subsequent actions; ensure the ordering of mockReturnValueOnce then mockReturnValue is preserved to match the component's selector call sequence.
54-54: Consider using async/await instead of thedonecallback pattern.The test uses the older
donecallback pattern. Modern Jest best practices recommend usingasync/awaitfor async tests, which provides cleaner error handling and more readable code.♻️ Suggested refactor
-test('Can display create wizard and create custom Deb ACS', async (done) => { +test('Can display create wizard and create custom Deb ACS', async () => {And at the end of the test:
- act(done); + // assertions complete });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.js` at line 54, The test "Can display create wizard and create custom Deb ACS" currently uses the done callback; refactor it to use async/await by removing the done parameter, awaiting all asynchronous calls (e.g., any render, userEvent, fetch/mocked API calls, or waitFor calls) and avoiding calling done() — ensure the test function signature is "test(..., async () => { ... })", replace any done() usages with awaited promises or await expect(...) assertions, and keep any cleanup/assertion logic after the awaited operations to preserve behavior.test/actions/pulp3/orchestration/alternate_content_source_create_test.rb (1)
63-70: Assert the DEB PRNs too.The YUM and file cases verify both
*_hrefand*_prn, but this new DEB case only checks the href prefixes. A regression in DEB PRN extraction would still pass here.Suggested assertions
assert_equal 1, `@deb_acs.smart_proxy_alternate_content_sources.count` assert `@deb_acs.smart_proxy_alternate_content_sources.first.remote_href.start_with`?('/pulp/api/v3/remotes/deb/apt/') assert `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_href.start_with`?('/pulp/api/v3/acs/deb/deb/') + assert_not_nil `@deb_acs.smart_proxy_alternate_content_sources.first.remote_prn` + assert_match(/^prn:deb\.aptremote:[0-9a-f\-]+$/, `@deb_acs.smart_proxy_alternate_content_sources.first.remote_prn`) + assert_not_nil `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_prn` + assert_match(/^prn:deb\.aptalternatecontentsource:[0-9a-f\-]+$/, `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_prn`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/orchestration/alternate_content_source_create_test.rb` around lines 63 - 70, The DEB test (test_deb_create) currently only asserts href prefixes; add assertions for the PRN fields on the created smart_proxy_alternate_content_sources to match the YUM/file tests. After creating smart_proxy_acs and syncing (in test_deb_create), assert that smart_proxy_alternate_content_sources.first.remote_href_prn starts_with the expected DEB remote PRN prefix and that smart_proxy_alternate_content_sources.first.alternate_content_source_prn starts_with the expected DEB ACS PRN prefix, using the same object references (`@deb_acs` and the smart_proxy_alternate_content_sources collection) as in the test.test/actions/pulp3/orchestration/alternate_content_source_update_test.rb (1)
187-189: Inconsistent assertion argument order.The assertion order here (
expected, actual) differs from similar tests in this file (Lines 115-117, 134-136) which use (actual, expected). While both work, consistency improves readability.♻️ Optional fix for consistency
- assert_equal proxy.url, remote_options[:proxy_url] - assert_equal proxy.username, remote_options[:proxy_username] - assert_equal proxy.password, remote_options[:proxy_password] + assert_equal remote_options[:proxy_url], proxy.url + assert_equal remote_options[:proxy_username], proxy.username + assert_equal remote_options[:proxy_password], proxy.password🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb` around lines 187 - 189, The three assertions comparing proxy fields to remote_options use (expected, actual) while the rest of the file uses (actual, expected); update the assertions that reference proxy.url/username/password and remote_options[:proxy_url]/[:proxy_username]/[:proxy_password] to use the same argument order as the other tests (i.e., swap the arguments so remote_options[...] is the first arg and proxy.* is the second) to keep assertion ordering consistent.test/models/katello/alternate_content_source_test.rb (1)
211-216: Redundantsave!call.The fixture is already loaded and persisted via
katello_alternate_content_sourcesin the setup block (Lines 167-170). The explicitsave!on Line 213 is unnecessary.♻️ Optional cleanup
def test_search_content_type_deb deb_acs = katello_alternate_content_sources(:deb_alternate_content_source) - deb_acs.save! acss = AlternateContentSource.search_for('content_type = "deb"') assert_includes acss, deb_acs end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/katello/alternate_content_source_test.rb` around lines 211 - 216, The test method test_search_content_type_deb contains a redundant call to save! on the fixture object katello_alternate_content_sources(:deb_alternate_content_source); remove the unnecessary deb_acs.save! invocation so the test simply retrieves the fixture and asserts it is included in AlternateContentSource.search_for('content_type = "deb"') without re-persisting the fixture.test/actions/pulp3/orchestration/deb_sync_test.rb (1)
28-35: Prefer scoped teardown cleanup here.
destroy_allon the wholeKatello::SmartProxyAlternateContentSourcetable is broader than this test needs. Scoping cleanup to@deb_acskeeps the teardown local and easier to reason about.🧹 Safer cleanup
- ::Katello::SmartProxyAlternateContentSource.destroy_all + ::Katello::SmartProxyAlternateContentSource.where( + alternate_content_source_id: `@deb_acs.id`, + smart_proxy_id: `@primary.id` + ).destroy_all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/orchestration/deb_sync_test.rb` around lines 28 - 35, The teardown is calling ::Katello::SmartProxyAlternateContentSource.destroy_all which clears the whole table; change it to only remove records related to this test by scoping to `@deb_acs` (e.g., call something like `@deb_acs.each` { |sma| sma.destroy } or ::Katello::SmartProxyAlternateContentSource.where(id: `@deb_acs.map`(&:id)).destroy_all) so the cleanup only affects the AlternateContentSource records created for this test; update the teardown method (the list iteration and final destroy_all) to use the scoped destroy based on `@deb_acs/its` ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/katello/alternate_content_source.rb`:
- Around line 181-186: In deb_constraints, the validation currently uses
deb_releases.to_s.strip.blank? which accepts separator-only strings like ",";
change the check to split deb_releases on commas, strip each element and reject
blank entries, and add the error when the resulting array is empty (e.g. use
deb_releases.to_s.split(',').map(&:strip).reject(&:blank?).empty?); update the
validation in the deb_constraints method (and the errors.add(:deb_releases, ...)
call) so separator-only values are rejected for custom? && deb?.
In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb`:
- Around line 38-41: The teardown loop deleting
smart_proxy_alternate_content_sources for `@deb_acs` does not refresh the `@deb_acs`
associations afterward; after calling
ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Delete,
smart_proxy_acs) for each smart_proxy_acs, call `@deb_acs.reload` to refresh
associations (matching other ACS cleanup blocks) so subsequent assertions
operate on the updated state.
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb`:
- Around line 320-337: The test_update_deb currently doesn't include
DEB-specific params so it won't detect if deb_releases, deb_components or
deb_architectures stop being permitted; update the PUT request in
test_update_deb to explicitly pass `@deb_acs.deb_releases`,
`@deb_acs.deb_components` and `@deb_acs.deb_architectures` in params (e.g.
deb_releases: ..., deb_components: ..., deb_architectures: ...) and keep the
surrounding assert_sync_task(::Actions::Katello::AlternateContentSource::Update)
and response/template assertions unchanged so the test will fail if those
attributes are not allowed through the controller.
In `@webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js`:
- Around line 62-66: The urlAndPathsValid function currently includes
subPathValidated in baseOk even when subpaths are hidden in DEB custom mode,
which can block the wizard; change the logic so subPathValidated is only
required when subpaths are visible. Concretely, inside urlAndPathsValid compute
a subPathOk that is true when subpaths are not shown (e.g., when contentType ===
'deb' && acsType === 'custom') and otherwise check subPathValidated !== 'error';
then set baseOk to use url !== '' && urlValidated !== 'error' && subPathOk so
hidden subpaths no longer block validation.
In `@webpack/scenes/AlternateContentSources/helpers.js`:
- Around line 30-33: The function declaration for spaceSep is missing a trailing
semicolon; update the end of the spaceSep declaration (the function that calls
toList and returns arr.join(' ') or emptyValue) to include a semicolon after the
closing brace to match the file's semicolon style and maintain consistent
formatting.
---
Outside diff comments:
In `@app/services/katello/pulp3/smart_proxy_mirror_repository.rb`:
- Around line 140-167: The method delete_orphan_alternate_content_sources builds
a local tasks array but never returns it; update
delete_orphan_alternate_content_sources to return the tasks array (same behavior
as delete_orphan_repositories / delete_orphan_distributions /
delete_orphan_remotes) so callers can receive the created task references from
the file_acs_api/yum_acs_api/deb_acs_api delete calls.
---
Nitpick comments:
In `@test/actions/pulp3/orchestration/alternate_content_source_create_test.rb`:
- Around line 63-70: The DEB test (test_deb_create) currently only asserts href
prefixes; add assertions for the PRN fields on the created
smart_proxy_alternate_content_sources to match the YUM/file tests. After
creating smart_proxy_acs and syncing (in test_deb_create), assert that
smart_proxy_alternate_content_sources.first.remote_href_prn starts_with the
expected DEB remote PRN prefix and that
smart_proxy_alternate_content_sources.first.alternate_content_source_prn
starts_with the expected DEB ACS PRN prefix, using the same object references
(`@deb_acs` and the smart_proxy_alternate_content_sources collection) as in the
test.
In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb`:
- Around line 187-189: The three assertions comparing proxy fields to
remote_options use (expected, actual) while the rest of the file uses (actual,
expected); update the assertions that reference proxy.url/username/password and
remote_options[:proxy_url]/[:proxy_username]/[:proxy_password] to use the same
argument order as the other tests (i.e., swap the arguments so
remote_options[...] is the first arg and proxy.* is the second) to keep
assertion ordering consistent.
In `@test/actions/pulp3/orchestration/deb_sync_test.rb`:
- Around line 28-35: The teardown is calling
::Katello::SmartProxyAlternateContentSource.destroy_all which clears the whole
table; change it to only remove records related to this test by scoping to
`@deb_acs` (e.g., call something like `@deb_acs.each` { |sma| sma.destroy } or
::Katello::SmartProxyAlternateContentSource.where(id:
`@deb_acs.map`(&:id)).destroy_all) so the cleanup only affects the
AlternateContentSource records created for this test; update the teardown method
(the list iteration and final destroy_all) to use the scoped destroy based on
`@deb_acs/its` ids.
In `@test/models/katello/alternate_content_source_test.rb`:
- Around line 211-216: The test method test_search_content_type_deb contains a
redundant call to save! on the fixture object
katello_alternate_content_sources(:deb_alternate_content_source); remove the
unnecessary deb_acs.save! invocation so the test simply retrieves the fixture
and asserts it is included in AlternateContentSource.search_for('content_type =
"deb"') without re-persisting the fixture.
In
`@webpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.js`:
- Around line 102-105: The test sets up the reactRedux.useSelector mock
(useSelectorMock) after interactions, which is fragile; move the
jest.spyOn(reactRedux, 'useSelector') and its
mockReturnValueOnce(smartProxyResult).mockReturnValue(contentCredentialResult.results)
setup to run before rendering or user interactions (e.g., at the start of the
test or in a beforeEach) so useSelector returns predictable values during render
and subsequent actions; ensure the ordering of mockReturnValueOnce then
mockReturnValue is preserved to match the component's selector call sequence.
- Line 54: The test "Can display create wizard and create custom Deb ACS"
currently uses the done callback; refactor it to use async/await by removing the
done parameter, awaiting all asynchronous calls (e.g., any render, userEvent,
fetch/mocked API calls, or waitFor calls) and avoiding calling done() — ensure
the test function signature is "test(..., async () => { ... })", replace any
done() usages with awaited promises or await expect(...) assertions, and keep
any cleanup/assertion logic after the awaited operations to preserve 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: 62180d4f-b071-4ca7-b9c6-853d5e8f8036
📒 Files selected for processing (32)
app/controllers/katello/api/v2/alternate_content_sources_controller.rbapp/models/katello/alternate_content_source.rbapp/services/katello/pulp3/alternate_content_source.rbapp/services/katello/pulp3/api/apt.rbapp/services/katello/pulp3/smart_proxy_mirror_repository.rbapp/views/katello/api/v2/alternate_content_sources/base.json.rabldb/migrate/20251110000001_add_deb_fields_to_acs.rbtest/actions/pulp3/orchestration/alternate_content_source_create_test.rbtest/actions/pulp3/orchestration/alternate_content_source_delete_test.rbtest/actions/pulp3/orchestration/alternate_content_source_refresh_test.rbtest/actions/pulp3/orchestration/alternate_content_source_update_test.rbtest/actions/pulp3/orchestration/deb_sync_test.rbtest/controllers/api/v2/alternate_content_sources_controller_test.rbtest/fixtures/models/katello_alternate_content_sources.ymltest/fixtures/models/katello_repositories.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.ymltest/fixtures/vcr_cassettes/actions/pulp3/apt_sync/sync_with_deb_acs.ymltest/models/katello/alternate_content_source_test.rbwebpack/scenes/AlternateContentSources/Create/ACSCreateWizard.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSReview.jswebpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.jswebpack/scenes/AlternateContentSources/Create/Steps/SelectSource.jswebpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.jswebpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.jswebpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.jswebpack/scenes/AlternateContentSources/helpers.js
Comment on lines
+38
to
+41
| @deb_acs.smart_proxy_alternate_content_sources.each do |smart_proxy_acs| | ||
| ForemanTasks.sync_task( | ||
| ::Actions::Pulp3::Orchestration::AlternateContentSource::Delete, smart_proxy_acs) | ||
| end |
There was a problem hiding this comment.
Missing @deb_acs.reload after teardown cleanup.
Other ACS cleanup blocks in teardown call reload to refresh associations (e.g., Lines 30, 36, 47, 53). This block is missing that call for consistency.
🔧 Proposed fix
`@deb_acs.smart_proxy_alternate_content_sources.each` do |smart_proxy_acs|
ForemanTasks.sync_task(
::Actions::Pulp3::Orchestration::AlternateContentSource::Delete, smart_proxy_acs)
end
+ `@deb_acs.reload`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @deb_acs.smart_proxy_alternate_content_sources.each do |smart_proxy_acs| | |
| ForemanTasks.sync_task( | |
| ::Actions::Pulp3::Orchestration::AlternateContentSource::Delete, smart_proxy_acs) | |
| end | |
| `@deb_acs.smart_proxy_alternate_content_sources.each` do |smart_proxy_acs| | |
| ForemanTasks.sync_task( | |
| ::Actions::Pulp3::Orchestration::AlternateContentSource::Delete, smart_proxy_acs) | |
| end | |
| `@deb_acs.reload` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb`
around lines 38 - 41, The teardown loop deleting
smart_proxy_alternate_content_sources for `@deb_acs` does not refresh the `@deb_acs`
associations afterward; after calling
ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Delete,
smart_proxy_acs) for each smart_proxy_acs, call `@deb_acs.reload` to refresh
associations (matching other ACS cleanup blocks) so subsequent assertions
operate on the updated state.
Comment on lines
+320
to
+337
| def test_update_deb | ||
| assert_sync_task(::Actions::Katello::AlternateContentSource::Update) do |acs, smart_proxies| | ||
| assert_equal acs.attributes.except('id', 'label'), @deb_acs.attributes.except('id', 'label') | ||
| assert_equal [@smart_proxy.id], smart_proxies.pluck(:id) | ||
| end | ||
|
|
||
| put :update, params: { | ||
| id: @deb_acs.id, | ||
| name: @deb_acs.name, | ||
| smart_proxy_ids: [@smart_proxy.id], | ||
| content_type: @deb_acs.content_type, | ||
| base_url: @deb_acs.base_url, | ||
| subpaths: @deb_acs.subpaths, | ||
| alternate_content_source_type: @deb_acs.alternate_content_source_type, | ||
| verify_ssl: @deb_acs.verify_ssl, | ||
| } | ||
| assert_response :success | ||
| assert_template 'api/v2/alternate_content_sources/show' |
There was a problem hiding this comment.
Exercise the DEB fields in the update request.
Because @deb_acs is already persisted with the Debian attributes, this assertion still passes even if update stops permitting deb_releases, deb_components, or deb_architectures. The request needs to touch those fields explicitly.
🔧 Suggested test adjustment
put :update, params: {
id: `@deb_acs.id`,
name: `@deb_acs.name`,
smart_proxy_ids: [`@smart_proxy.id`],
content_type: `@deb_acs.content_type`,
base_url: `@deb_acs.base_url`,
subpaths: `@deb_acs.subpaths`,
+ deb_releases: `@deb_acs.deb_releases`,
+ deb_components: `@deb_acs.deb_components`,
+ deb_architectures: `@deb_acs.deb_architectures`,
alternate_content_source_type: `@deb_acs.alternate_content_source_type`,
verify_ssl: `@deb_acs.verify_ssl`,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_update_deb | |
| assert_sync_task(::Actions::Katello::AlternateContentSource::Update) do |acs, smart_proxies| | |
| assert_equal acs.attributes.except('id', 'label'), @deb_acs.attributes.except('id', 'label') | |
| assert_equal [@smart_proxy.id], smart_proxies.pluck(:id) | |
| end | |
| put :update, params: { | |
| id: @deb_acs.id, | |
| name: @deb_acs.name, | |
| smart_proxy_ids: [@smart_proxy.id], | |
| content_type: @deb_acs.content_type, | |
| base_url: @deb_acs.base_url, | |
| subpaths: @deb_acs.subpaths, | |
| alternate_content_source_type: @deb_acs.alternate_content_source_type, | |
| verify_ssl: @deb_acs.verify_ssl, | |
| } | |
| assert_response :success | |
| assert_template 'api/v2/alternate_content_sources/show' | |
| def test_update_deb | |
| assert_sync_task(::Actions::Katello::AlternateContentSource::Update) do |acs, smart_proxies| | |
| assert_equal acs.attributes.except('id', 'label'), `@deb_acs.attributes.except`('id', 'label') | |
| assert_equal [`@smart_proxy.id`], smart_proxies.pluck(:id) | |
| end | |
| put :update, params: { | |
| id: `@deb_acs.id`, | |
| name: `@deb_acs.name`, | |
| smart_proxy_ids: [`@smart_proxy.id`], | |
| content_type: `@deb_acs.content_type`, | |
| base_url: `@deb_acs.base_url`, | |
| subpaths: `@deb_acs.subpaths`, | |
| deb_releases: `@deb_acs.deb_releases`, | |
| deb_components: `@deb_acs.deb_components`, | |
| deb_architectures: `@deb_acs.deb_architectures`, | |
| alternate_content_source_type: `@deb_acs.alternate_content_source_type`, | |
| verify_ssl: `@deb_acs.verify_ssl`, | |
| } | |
| assert_response :success | |
| assert_template 'api/v2/alternate_content_sources/show' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb` around
lines 320 - 337, The test_update_deb currently doesn't include DEB-specific
params so it won't detect if deb_releases, deb_components or deb_architectures
stop being permitted; update the PUT request in test_update_deb to explicitly
pass `@deb_acs.deb_releases`, `@deb_acs.deb_components` and
`@deb_acs.deb_architectures` in params (e.g. deb_releases: ..., deb_components:
..., deb_architectures: ...) and keep the surrounding
assert_sync_task(::Actions::Katello::AlternateContentSource::Update) and
response/template assertions unchanged so the test will fail if those attributes
are not allowed through the controller.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/katello/api/v2/alternate_content_sources_controller.rb (1)
41-49:⚠️ Potential issue | 🔴 CriticalLines 41-49 reference undefined
@acsvariable, causing NoMethodError on CSV export.The
indexaction references@acs.custom?,@acs.rhui?, and@acs.simplified?at lines 41 and 45, but@acsis never assigned. Thefind_authorized_katello_resourcebefore_action (line 7) explicitly excludes theindexaction, so there's no initialization. This will raiseNoMethodErrorwhen attempting to export to CSV.Additionally, the logic appears flawed: the code retrieves a collection of
alternate_content_sources(multiple records) but then checks properties of a single undefined@acsobject. The conditional logic needs redesign—either filter the collection before export, or iterate through records checking each one's type individually.
♻️ Duplicate comments (3)
webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js (1)
62-67:⚠️ Potential issue | 🟡 MinorSubpath validation still blocks wizard in DEB mode when subpaths are hidden.
The
urlAndPathsValid()function includessubPathValidated !== 'error'inbaseOkunconditionally. If a user enters invalid subpaths in a non-DEB flow and then switches to DEB (where subpaths are not shown), the wizard can remain blocked on a field that is no longer visible or editable.Suggested fix
const urlAndPathsValid = () => { - const baseOk = url !== '' && urlValidated !== 'error' && subPathValidated !== 'error'; + const subpathsOk = contentType === 'deb' || subPathValidated !== 'error'; + const baseOk = url !== '' && urlValidated !== 'error' && subpathsOk; const isDebCustom = contentType === 'deb' && acsType === 'custom'; const hasReleases = (debReleases || '').trim().split(/[,\s]+/).filter(Boolean).length > 0; return baseOk && (!isDebCustom || hasReleases); };app/models/katello/alternate_content_source.rb (1)
184-185:⚠️ Potential issue | 🟠 MajorReject separator-only
deb_releasesvalues.The current blank check allows values like
","/" , "to pass validation, which can persist invalid DEB ACS definitions and fail later in orchestration.🔧 Proposed fix
- if custom? && deb_releases.to_s.strip.blank? + releases = deb_releases.to_s.split(',').map(&:strip).reject(&:blank?) + if custom? && releases.empty? errors.add(:deb_releases, 'must be provided for deb alternate content sources') end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/katello/alternate_content_source.rb` around lines 184 - 185, The validation for deb_releases currently only checks blank? which lets separator-only strings like "," pass; update the condition in the validation block (the custom? check that calls errors.add on :deb_releases) to treat comma-separated values with only whitespace/empty entries as blank by splitting deb_releases on "," then mapping strip and rejecting blank entries, and add the error if the resulting array is empty (i.e. no real release tokens).test/controllers/api/v2/alternate_content_sources_controller_test.rb (1)
326-335:⚠️ Potential issue | 🟡 MinorPass the DEB fields in the update request.
This still passes if
deb_releases,deb_components, ordeb_architecturesstop being permitted, because the expectation compares against the already-persisted fixture. Include them in the PUT payload so the controller whitelist is actually exercised.🔧 Suggested test adjustment
put :update, params: { id: `@deb_acs.id`, name: `@deb_acs.name`, smart_proxy_ids: [`@smart_proxy.id`], content_type: `@deb_acs.content_type`, base_url: `@deb_acs.base_url`, subpaths: `@deb_acs.subpaths`, + deb_releases: `@deb_acs.deb_releases`, + deb_components: `@deb_acs.deb_components`, + deb_architectures: `@deb_acs.deb_architectures`, alternate_content_source_type: `@deb_acs.alternate_content_source_type`, verify_ssl: `@deb_acs.verify_ssl`, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb` around lines 326 - 335, The test sends an update via put :update for `@deb_acs` but doesn't include the DEB-specific fields, so the controller's strong-parameter whitelist isn't exercised; update the params in the put :update call (in test/controllers/api/v2/alternate_content_sources_controller_test.rb, the put :update block that references `@deb_acs`) to include deb_releases, deb_components, and deb_architectures (populated from `@deb_acs.deb_releases`, `@deb_acs.deb_components`, `@deb_acs.deb_architectures`) so the request payload actually contains those attributes and will fail if they are no longer permitted.
🧹 Nitpick comments (2)
db/migrate/20251110000001_add_deb_fields_to_acs.rb (1)
3-5: Consider addinglimit: 255to match the existing deb fields migration, or use:textfor consistency across both tables.The existing migration for deb fields in
katello_repositories(20170720142145) uses:string, limit: 255fordeb_releases,deb_components, anddeb_architectures. The new ACS migration should either match that pattern with an explicit limit or adopt:textuniversally. Test data shows no values approaching 255 characters, but clarifying the intended field size strategy is important to avoid future truncation risks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20251110000001_add_deb_fields_to_acs.rb` around lines 3 - 5, The new migration adds deb_releases, deb_components, and deb_architectures to katello_alternate_content_sources without a size specification; update the three add_column calls to either add limit: 255 (to match the existing katello_repositories migration) like add_column :katello_alternate_content_sources, :deb_releases, :string, limit: 255 (and same for deb_components and deb_architectures) or change the column type to :text for all three for consistency across tables—pick one approach and apply it consistently to the three add_column lines.webpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.js (1)
362-404: Consider defensive check forsubpathsbefore calling.join().Line 371 calls
subpaths.join()without a null check. Whilesubpathsshould be an array from the API for custom/rhui types, a defensive check would prevent runtime errors if the API returns unexpected data.Suggested fix
<TextListItem aria-label="subpaths_text_value" component={TextListItemVariants.dd} > - {subpaths.join()} + {subpaths?.join() || <InactiveText text="N/A" />} </TextListItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.js` around lines 362 - 404, subpaths may be null/undefined before calling .join() in the ACSExpandableDetails component where subpaths.join() is rendered; update the rendering logic to defensively check that subpaths is an array (e.g., using Array.isArray(subpaths)) and only call .join() when true, otherwise render a fallback such as <InactiveText text="N/A" /> or an empty string; ensure the change targets the TextListItem with aria-label="subpaths_text_value" so other debMode branches are untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/katello/pulp3/smart_proxy_mirror_repository.rb`:
- Around line 160-167: The method delete_orphan_alternate_content_sources
currently may return nil when the deb branch is not executed; ensure it always
returns the accumulated tasks array by adding an explicit tasks return at the
end of the method. Locate the delete_orphan_alternate_content_sources method and
append a final expression returning tasks (matching the behavior of
delete_orphan_remotes) so callers consistently receive the tasks array even when
RepositoryTypeManager.enabled_repository_types['deb'] is false.
In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb`:
- Around line 82-92: The test_deb_update currently only mutates and asserts
deb_releases; update the test (test_deb_update) to also mutate deb_components
and deb_architectures on `@deb_acs` before calling ForemanTasks.sync_task for
Actions::Pulp3::Orchestration::AlternateContentSource::Update, then fetch
pulp_remote via `@deb_acs.backend_service`(`@primary`).get_remote and add assertions
that pulp_remote.components matches the new deb_components and
pulp_remote.architectures matches the new deb_architectures (alongside the
existing assertions for base_url and distributions) to ensure all DEB remote
fields are covered.
In `@test/actions/pulp3/orchestration/deb_sync_test.rb`:
- Around line 7-8: The test's setup mutates global User.current to users(:admin)
but never restores it; modify setup to save the prior user (e.g., previous_user
= User.current) and then update teardown to restore User.current = previous_user
before performing the rest of teardown cleanup so later tests don't inherit
admin context; reference the setup and teardown methods and the User.current
assignment when making the change.
In
`@test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.yml`:
- Around line 8-247: This cassette contains secret material (embedded certs/keys
and upstream credentials) inside fields such as base64_string (which decodes to
ca_cert and client_key), client_key, ca_cert, proxy_username, proxy_password and
the Authorization header; update the VCR/filtering logic to replace those values
with placeholders (e.g. <REDACTED_CA_CERT>, <REDACTED_CLIENT_KEY>,
<REDACTED_PROXY_USER>, <REDACTED_PROXY_PASS>, <REDACTED_AUTH>) when recording
(look for the vcr config or filter hooks used for these fixtures and the code
that writes/reads this cassette), then rerecord the cassette so the committed
YAML contains the placeholders instead of real secrets.
In
`@test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.yml`:
- Around line 72-116: The cassette contains sensitive fields (hidden_fields
array values like "client_key", "username", "password") stored in base64; update
the test VCR setup to add filter_sensitive_data rules that mask these fields
before recording (e.g., add VCR.filter_sensitive_data for client_key, username,
password and any tcls_validation token) and re-record the cassette so the
recorded payload has placeholders instead of real credentials; target the test
helper that configures VCR (look for VCR.configure / filter_sensitive_data
usage) and add filters for the base64-encoded RSA key and credentials then
re-run the tests to regenerate the cassette.
In
`@webpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.js`:
- Around line 54-55: The test "Can display create wizard and create custom Deb
ACS" mixes callback and promise styles: remove the done parameter from the test
function signature and eliminate any act(done) calls (and any other calls to
done) inside the test so the async function relies on returned Promises/await;
replace act(done) with await act(...) or await when resolving effects so the
async test uses only async/await semantics (look for the test string and any
act(done) occurrences to update).
In
`@webpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.js`:
- Around line 56-63: The current debMode branch sets params.subpaths to an empty
array which wipes hidden Debian subpaths; instead, remove the explicit subpaths:
[] assignment so that when debMode is true the params object does not include
subpaths and the server retains existing values—update the block building params
(the code referencing debMode, params,
deb_releases/deb_components/deb_architectures and spaceSepOrEmpty) to omit
subpaths rather than set it to [].
---
Duplicate comments:
In `@app/models/katello/alternate_content_source.rb`:
- Around line 184-185: The validation for deb_releases currently only checks
blank? which lets separator-only strings like "," pass; update the condition in
the validation block (the custom? check that calls errors.add on :deb_releases)
to treat comma-separated values with only whitespace/empty entries as blank by
splitting deb_releases on "," then mapping strip and rejecting blank entries,
and add the error if the resulting array is empty (i.e. no real release tokens).
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb`:
- Around line 326-335: The test sends an update via put :update for `@deb_acs` but
doesn't include the DEB-specific fields, so the controller's strong-parameter
whitelist isn't exercised; update the params in the put :update call (in
test/controllers/api/v2/alternate_content_sources_controller_test.rb, the put
:update block that references `@deb_acs`) to include deb_releases, deb_components,
and deb_architectures (populated from `@deb_acs.deb_releases`,
`@deb_acs.deb_components`, `@deb_acs.deb_architectures`) so the request payload
actually contains those attributes and will fail if they are no longer
permitted.
---
Nitpick comments:
In `@db/migrate/20251110000001_add_deb_fields_to_acs.rb`:
- Around line 3-5: The new migration adds deb_releases, deb_components, and
deb_architectures to katello_alternate_content_sources without a size
specification; update the three add_column calls to either add limit: 255 (to
match the existing katello_repositories migration) like add_column
:katello_alternate_content_sources, :deb_releases, :string, limit: 255 (and same
for deb_components and deb_architectures) or change the column type to :text for
all three for consistency across tables—pick one approach and apply it
consistently to the three add_column lines.
In `@webpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.js`:
- Around line 362-404: subpaths may be null/undefined before calling .join() in
the ACSExpandableDetails component where subpaths.join() is rendered; update the
rendering logic to defensively check that subpaths is an array (e.g., using
Array.isArray(subpaths)) and only call .join() when true, otherwise render a
fallback such as <InactiveText text="N/A" /> or an empty string; ensure the
change targets the TextListItem with aria-label="subpaths_text_value" so other
debMode branches are untouched.
🪄 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: 073ec419-cf1d-4770-a0f9-7819150c3d70
📒 Files selected for processing (32)
app/controllers/katello/api/v2/alternate_content_sources_controller.rbapp/models/katello/alternate_content_source.rbapp/services/katello/pulp3/alternate_content_source.rbapp/services/katello/pulp3/api/apt.rbapp/services/katello/pulp3/smart_proxy_mirror_repository.rbapp/views/katello/api/v2/alternate_content_sources/base.json.rabldb/migrate/20251110000001_add_deb_fields_to_acs.rbtest/actions/pulp3/orchestration/alternate_content_source_create_test.rbtest/actions/pulp3/orchestration/alternate_content_source_delete_test.rbtest/actions/pulp3/orchestration/alternate_content_source_refresh_test.rbtest/actions/pulp3/orchestration/alternate_content_source_update_test.rbtest/actions/pulp3/orchestration/deb_sync_test.rbtest/controllers/api/v2/alternate_content_sources_controller_test.rbtest/fixtures/models/katello_alternate_content_sources.ymltest/fixtures/models/katello_repositories.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.ymltest/fixtures/vcr_cassettes/actions/pulp3/apt_sync/sync_with_deb_acs.ymltest/models/katello/alternate_content_source_test.rbwebpack/scenes/AlternateContentSources/Create/ACSCreateWizard.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSReview.jswebpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.jswebpack/scenes/AlternateContentSources/Create/Steps/SelectSource.jswebpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.jswebpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.jswebpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.jswebpack/scenes/AlternateContentSources/helpers.js
Comment on lines
+82
to
+92
| def test_deb_update | ||
| ::Katello::Pulp3::AlternateContentSource.any_instance.stubs(:generate_backend_object_name).returns(@deb_acs.name) | ||
| smart_proxy_acs = ::Katello::SmartProxyAlternateContentSource.create(alternate_content_source_id: @deb_acs.id, smart_proxy_id: @primary.id) | ||
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Create, smart_proxy_acs) | ||
| @deb_acs.update(base_url: 'https://fixtures.pulpproject.org/debian/', deb_releases: 'ragnarok ginnungagap') | ||
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Update, smart_proxy_acs) | ||
|
|
||
| pulp_remote = @deb_acs.backend_service(@primary).get_remote | ||
| assert_equal @deb_acs.base_url, pulp_remote.url | ||
| assert_equal 'ragnarok ginnungagap', pulp_remote.distributions | ||
| end |
There was a problem hiding this comment.
Cover all DEB remote fields in the update test.
This only mutates/asserts deb_releases, so a regression in deb_components or deb_architectures mapping would still pass here.
🔧 Suggested test expansion
- `@deb_acs.update`(base_url: 'https://fixtures.pulpproject.org/debian/', deb_releases: 'ragnarok ginnungagap')
+ `@deb_acs.update`!(
+ base_url: 'https://fixtures.pulpproject.org/debian/',
+ deb_releases: 'ragnarok ginnungagap',
+ deb_components: 'main contrib',
+ deb_architectures: 'amd64 arm64'
+ )
ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Update, smart_proxy_acs)
pulp_remote = `@deb_acs.backend_service`(`@primary`).get_remote
assert_equal `@deb_acs.base_url`, pulp_remote.url
- assert_equal 'ragnarok ginnungagap', pulp_remote.distributions
+ assert_equal `@deb_acs.deb_releases`, pulp_remote.distributions
+ assert_equal `@deb_acs.deb_components`, pulp_remote.components
+ assert_equal `@deb_acs.deb_architectures`, pulp_remote.architectures📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_deb_update | |
| ::Katello::Pulp3::AlternateContentSource.any_instance.stubs(:generate_backend_object_name).returns(@deb_acs.name) | |
| smart_proxy_acs = ::Katello::SmartProxyAlternateContentSource.create(alternate_content_source_id: @deb_acs.id, smart_proxy_id: @primary.id) | |
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Create, smart_proxy_acs) | |
| @deb_acs.update(base_url: 'https://fixtures.pulpproject.org/debian/', deb_releases: 'ragnarok ginnungagap') | |
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Update, smart_proxy_acs) | |
| pulp_remote = @deb_acs.backend_service(@primary).get_remote | |
| assert_equal @deb_acs.base_url, pulp_remote.url | |
| assert_equal 'ragnarok ginnungagap', pulp_remote.distributions | |
| end | |
| def test_deb_update | |
| ::Katello::Pulp3::AlternateContentSource.any_instance.stubs(:generate_backend_object_name).returns(`@deb_acs.name`) | |
| smart_proxy_acs = ::Katello::SmartProxyAlternateContentSource.create(alternate_content_source_id: `@deb_acs.id`, smart_proxy_id: `@primary.id`) | |
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Create, smart_proxy_acs) | |
| `@deb_acs.update`!( | |
| base_url: 'https://fixtures.pulpproject.org/debian/', | |
| deb_releases: 'ragnarok ginnungagap', | |
| deb_components: 'main contrib', | |
| deb_architectures: 'amd64 arm64' | |
| ) | |
| ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Update, smart_proxy_acs) | |
| pulp_remote = `@deb_acs.backend_service`(`@primary`).get_remote | |
| assert_equal `@deb_acs.base_url`, pulp_remote.url | |
| assert_equal `@deb_acs.deb_releases`, pulp_remote.distributions | |
| assert_equal `@deb_acs.deb_components`, pulp_remote.components | |
| assert_equal `@deb_acs.deb_architectures`, pulp_remote.architectures | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/actions/pulp3/orchestration/alternate_content_source_update_test.rb`
around lines 82 - 92, The test_deb_update currently only mutates and asserts
deb_releases; update the test (test_deb_update) to also mutate deb_components
and deb_architectures on `@deb_acs` before calling ForemanTasks.sync_task for
Actions::Pulp3::Orchestration::AlternateContentSource::Update, then fetch
pulp_remote via `@deb_acs.backend_service`(`@primary`).get_remote and add assertions
that pulp_remote.components matches the new deb_components and
pulp_remote.architectures matches the new deb_architectures (alongside the
existing assertions for base_url and distributions) to ensure all DEB remote
fields are covered.
Contributor
|
I went through these code rabbit reviews and left some comments on the ones I think we should fix. Can you take a look at the proposed fixes? |
ianballou
reviewed
Apr 7, 2026
Member
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
Handling some code rabbit reviews:
Member
|
We should be able to merge this after the small tasks comment is dealt with above. This change is looking great. |
Contributor
|
FYI: We decided we should add some more validation on the API controller, right now it will let me set subpaths via the API for a deb ACS so long as I don't set any smart proxies. If I then try to add a smart proxy to actually use it later the update action will fail, because Katello now tries to create the Pulp object with subpaths and Pulp will say nope. This is not a great user experience. It should simply fail on the Katello API validation, thus forcing me to create something sensible to begin with. |
Member
Sounds good to me, thanks for testing it, I didn't realize the subpaths would propagate in so far. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/controllers/api/v2/alternate_content_sources_controller_test.rb (1)
326-335:⚠️ Potential issue | 🟡 MinorPass the DEB fields through
test_update_deb.
@deb_acsalready hasdeb_releases,deb_components, anddeb_architecturespersisted, so this request still passes if update stops permitting those params. Include them in the PUT payload so the test covers the new controller surface.Suggested fix
put :update, params: { id: `@deb_acs.id`, name: `@deb_acs.name`, smart_proxy_ids: [`@smart_proxy.id`], content_type: `@deb_acs.content_type`, base_url: `@deb_acs.base_url`, subpaths: `@deb_acs.subpaths`, + deb_releases: `@deb_acs.deb_releases`, + deb_components: `@deb_acs.deb_components`, + deb_architectures: `@deb_acs.deb_architectures`, alternate_content_source_type: `@deb_acs.alternate_content_source_type`, verify_ssl: `@deb_acs.verify_ssl`, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb` around lines 326 - 335, The test_update_deb case's PUT request is missing explicit DEB params so it doesn't exercise the controller's new param handling; modify the params in the put :update call (inside test_update_deb) to include deb_releases, deb_components, and deb_architectures (e.g. pass `@deb_acs.deb_releases`, `@deb_acs.deb_components`, `@deb_acs.deb_architectures`) so the request payload covers the full DEB fields surface the controller now accepts.webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js (1)
62-66:⚠️ Potential issue | 🟠 MajorDon't let hidden subpaths block the DEB flow.
Line 63 still folds
subPathValidatedintobaseOkeven when the subpaths field is hidden. If someone enters invalid subpaths before switching to a DEB custom ACS, step 5 can stay blocked with no visible field to correct.Suggested fix
const urlAndPathsValid = () => { - const baseOk = url !== '' && urlValidated !== 'error' && subPathValidated !== 'error'; + const subpathsOk = contentType === 'deb' || subPathValidated !== 'error'; + const baseOk = url !== '' && urlValidated !== 'error' && subpathsOk; const isDebCustom = contentType === 'deb' && acsType === 'custom'; const hasReleases = (debReleases || '').trim().split(/[,\s]+/).filter(Boolean).length > 0; return baseOk && (!isDebCustom || hasReleases); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js` around lines 62 - 66, The validation currently treats subPathValidated as required even when the subpaths field is hidden; update urlAndPathsValid so baseOk only checks subPathValidated when the subpaths field is visible. Concretely, change the baseOk expression in urlAndPathsValid to use a visibility check (e.g. const subpathsVisible = !(contentType === 'deb' && acsType === 'custom')) and require subPathValidated !== 'error' only when subpathsVisible; keep the existing checks for url and urlValidated and leave the DEB custom release logic (isDebCustom / hasReleases) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.js`:
- Around line 69-70: The code currently does raw subpaths.split(',') which can
leave leading/trailing spaces and empty strings; update the ACSCreateFinish.js
branch that builds acsParams so that instead of using subpaths.split(',') it
normalizes tokens the same way ACSEditURLPaths.js does: split on commas, trim
each token, and filter out empty strings before assigning to acsParams
(reference the acsParams variable and the subpaths handling logic).
---
Duplicate comments:
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb`:
- Around line 326-335: The test_update_deb case's PUT request is missing
explicit DEB params so it doesn't exercise the controller's new param handling;
modify the params in the put :update call (inside test_update_deb) to include
deb_releases, deb_components, and deb_architectures (e.g. pass
`@deb_acs.deb_releases`, `@deb_acs.deb_components`, `@deb_acs.deb_architectures`) so
the request payload covers the full DEB fields surface the controller now
accepts.
In `@webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js`:
- Around line 62-66: The validation currently treats subPathValidated as
required even when the subpaths field is hidden; update urlAndPathsValid so
baseOk only checks subPathValidated when the subpaths field is visible.
Concretely, change the baseOk expression in urlAndPathsValid to use a visibility
check (e.g. const subpathsVisible = !(contentType === 'deb' && acsType ===
'custom')) and require subPathValidated !== 'error' only when subpathsVisible;
keep the existing checks for url and urlValidated and leave the DEB custom
release logic (isDebCustom / hasReleases) unchanged.
🪄 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: 35fbd9ec-a705-470c-870b-43701468a9cc
📒 Files selected for processing (32)
app/controllers/katello/api/v2/alternate_content_sources_controller.rbapp/models/katello/alternate_content_source.rbapp/services/katello/pulp3/alternate_content_source.rbapp/services/katello/pulp3/api/apt.rbapp/services/katello/pulp3/smart_proxy_mirror_repository.rbapp/views/katello/api/v2/alternate_content_sources/base.json.rabldb/migrate/20251110000001_add_deb_fields_to_acs.rbtest/actions/pulp3/orchestration/alternate_content_source_create_test.rbtest/actions/pulp3/orchestration/alternate_content_source_delete_test.rbtest/actions/pulp3/orchestration/alternate_content_source_refresh_test.rbtest/actions/pulp3/orchestration/alternate_content_source_update_test.rbtest/actions/pulp3/orchestration/deb_sync_test.rbtest/controllers/api/v2/alternate_content_sources_controller_test.rbtest/fixtures/models/katello_alternate_content_sources.ymltest/fixtures/models/katello_repositories.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.ymltest/fixtures/vcr_cassettes/actions/pulp3/apt_sync/sync_with_deb_acs.ymltest/models/katello/alternate_content_source_test.rbwebpack/scenes/AlternateContentSources/Create/ACSCreateWizard.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSReview.jswebpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.jswebpack/scenes/AlternateContentSources/Create/Steps/SelectSource.jswebpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.jswebpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.jswebpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.jswebpack/scenes/AlternateContentSources/helpers.js
✅ Files skipped from review due to trivial changes (14)
- webpack/scenes/AlternateContentSources/Create/Steps/SelectSource.js
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.yml
- test/fixtures/models/katello_repositories.yml
- test/fixtures/models/katello_alternate_content_sources.yml
- test/actions/pulp3/orchestration/alternate_content_source_update_test.rb
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.yml
- db/migrate/20251110000001_add_deb_fields_to_acs.rb
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.yml
- app/models/katello/alternate_content_source.rb
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.yml
- app/views/katello/api/v2/alternate_content_sources/base.json.rabl
- test/actions/pulp3/orchestration/deb_sync_test.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- app/services/katello/pulp3/smart_proxy_mirror_repository.rb
- app/services/katello/pulp3/api/apt.rb
- webpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.js
- webpack/scenes/AlternateContentSources/helpers.js
- app/services/katello/pulp3/alternate_content_source.rb
- webpack/scenes/AlternateContentSources/Create/tests/acsCreateDeb.test.js
- test/models/katello/alternate_content_source_test.rb
- test/actions/pulp3/orchestration/alternate_content_source_delete_test.rb
hstct
force-pushed
the
add-debian-acs
branch
2 times, most recently
from
97da682 to
615491b
Compare
Contributor
Author
|
ugh, I'll fix those tests asap |
Contributor
Thank you I think once those tests are fixed we can get this merged. @ianballou does that sound right? |
Member
I think it's worth briefly verifying what happens when you pass subpaths in now, but assuming that looks fine, yep, this can merge then. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js (1)
62-66:⚠️ Potential issue | 🟠 MajorDon’t gate Debian creation on hidden subpath validation.
Line 63 still requires
subPathValidated !== 'error', so a user can enter invalid subpaths in a non-Debian flow, switch to Debian, and be blocked by a hidden field. Skip subpath validation whencontentType === 'deb'.Proposed fix
const urlAndPathsValid = () => { - const baseOk = url !== '' && urlValidated !== 'error' && subPathValidated !== 'error'; + const subpathsOk = contentType === 'deb' || subPathValidated !== 'error'; + const baseOk = url !== '' && urlValidated !== 'error' && subpathsOk; const isDebCustom = contentType === 'deb' && acsType === 'custom'; const hasReleases = (debReleases || '').trim().split(/[,\s]+/).filter(Boolean).length > 0; return baseOk && (!isDebCustom || hasReleases); };Based on learnings, when in Debian (
deb) ACS mode, subpaths are intentionally cleared because Debian ACSs do not use subpaths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js` around lines 62 - 66, The urlAndPathsValid function currently requires subPathValidated !== 'error' for all content types, which incorrectly blocks Debian ACS creation when subpaths are intentionally cleared; update the logic in urlAndPathsValid to skip subPathValidated checks when contentType === 'deb' (i.e., treat subpath validation as not required for Debian), keeping the existing checks for url and urlValidated and preserving the deb-specific logic using acsType === 'custom' and debReleases to determine whether releases are present.test/controllers/api/v2/alternate_content_sources_controller_test.rb (1)
345-360:⚠️ Potential issue | 🟡 MinorExercise the Debian-specific fields in the update request.
test_update_debstill passes even if the controller stops permittingdeb_releases,deb_components, ordeb_architectures, because@deb_acsalready has those persisted values. Include them in thePUTparams so the test covers the update whitelist.Proposed test adjustment
put :update, params: { id: `@deb_acs.id`, name: `@deb_acs.name`, smart_proxy_ids: [`@smart_proxy.id`], content_type: `@deb_acs.content_type`, base_url: `@deb_acs.base_url`, subpaths: `@deb_acs.subpaths`, + deb_releases: `@deb_acs.deb_releases`, + deb_components: `@deb_acs.deb_components`, + deb_architectures: `@deb_acs.deb_architectures`, alternate_content_source_type: `@deb_acs.alternate_content_source_type`, verify_ssl: `@deb_acs.verify_ssl`, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb` around lines 345 - 360, The test_update_deb currently omits Debian-specific params so it doesn't exercise the controller's strong-params whitelist; update the PUT :update call in test_update_deb to include `@deb_acs.deb_releases`, `@deb_acs.deb_components`, and `@deb_acs.deb_architectures` (using the same keys deb_releases, deb_components, deb_architectures) alongside the existing params so the controller's Update action (and the assert_sync_task for ::Actions::Katello::AlternateContentSource::Update) actually verifies those fields are permitted and sent in the request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/katello/alternate_content_source.rb`:
- Around line 70-72: The :if option currently uses invalid Ruby syntax ("{
:custom? && !deb? }"); replace it with a Proc/lambda that evaluates the
predicate on the model instance (e.g. use a Proc or ->(r) { r.custom? && !r.deb?
}) so the validator Validators::AlternateContentSourcePathValidator on the model
(the validates_with call for attributes :base_url and :subpaths) only runs when
record.custom? && !record.deb?.
In `@app/services/katello/pulp3/alternate_content_source.rb`:
- Around line 18-20: The join_space_or_nil method currently treats the entire
input as a single value so inputs like "trixie, bookworm" remain
comma-separated; change its normalization to split string inputs on commas and
whitespace (e.g. using a regexp like /[,\s]+/), then map to strings, strip,
reject blank and join with a single space, returning nil for empty result; apply
the same fix to the analogous normalization function present around lines 70-73
so both places produce space-separated tokens sent to Pulp.
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb`:
- Line 154: Remove the trailing whitespace on the blank line that RuboCop
flagged in the alternate_content_sources_controller_test test file; open
test/controllers/api/v2/alternate_content_sources_controller_test.rb, locate the
empty line around the failing test block and delete any stray spaces so the line
is truly empty (no spaces or tabs), then save and re-run RuboCop.
---
Duplicate comments:
In `@test/controllers/api/v2/alternate_content_sources_controller_test.rb`:
- Around line 345-360: The test_update_deb currently omits Debian-specific
params so it doesn't exercise the controller's strong-params whitelist; update
the PUT :update call in test_update_deb to include `@deb_acs.deb_releases`,
`@deb_acs.deb_components`, and `@deb_acs.deb_architectures` (using the same keys
deb_releases, deb_components, deb_architectures) alongside the existing params
so the controller's Update action (and the assert_sync_task for
::Actions::Katello::AlternateContentSource::Update) actually verifies those
fields are permitted and sent in the request.
In `@webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js`:
- Around line 62-66: The urlAndPathsValid function currently requires
subPathValidated !== 'error' for all content types, which incorrectly blocks
Debian ACS creation when subpaths are intentionally cleared; update the logic in
urlAndPathsValid to skip subPathValidated checks when contentType === 'deb'
(i.e., treat subpath validation as not required for Debian), keeping the
existing checks for url and urlValidated and preserving the deb-specific logic
using acsType === 'custom' and debReleases to determine whether releases are
present.
🪄 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: 8ac4aeca-7f25-4dfc-a759-6762072c8e9a
📒 Files selected for processing (32)
app/controllers/katello/api/v2/alternate_content_sources_controller.rbapp/models/katello/alternate_content_source.rbapp/services/katello/pulp3/alternate_content_source.rbapp/services/katello/pulp3/api/apt.rbapp/services/katello/pulp3/smart_proxy_mirror_repository.rbapp/views/katello/api/v2/alternate_content_sources/base.json.rabldb/migrate/20251110000001_add_deb_fields_to_acs.rbtest/actions/pulp3/orchestration/alternate_content_source_create_test.rbtest/actions/pulp3/orchestration/alternate_content_source_delete_test.rbtest/actions/pulp3/orchestration/alternate_content_source_refresh_test.rbtest/actions/pulp3/orchestration/alternate_content_source_update_test.rbtest/actions/pulp3/orchestration/deb_sync_test.rbtest/controllers/api/v2/alternate_content_sources_controller_test.rbtest/fixtures/models/katello_alternate_content_sources.ymltest/fixtures/models/katello_repositories.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.ymltest/fixtures/vcr_cassettes/actions/pulp3/apt_sync/sync_with_deb_acs.ymltest/models/katello/alternate_content_source_test.rbwebpack/scenes/AlternateContentSources/Create/ACSCreateWizard.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSReview.jswebpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.jswebpack/scenes/AlternateContentSources/Create/Steps/SelectSource.jswebpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.jswebpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.jswebpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.jswebpack/scenes/AlternateContentSources/helpers.js
✅ Files skipped from review due to trivial changes (13)
- app/views/katello/api/v2/alternate_content_sources/base.json.rabl
- webpack/scenes/AlternateContentSources/Create/Steps/SelectSource.js
- test/fixtures/models/katello_repositories.yml
- db/migrate/20251110000001_add_deb_fields_to_acs.rb
- webpack/scenes/AlternateContentSources/Create/Steps/ACSReview.js
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.yml
- webpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.js
- webpack/scenes/AlternateContentSources/Create/tests/acsCreateDeb.test.js
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- webpack/scenes/AlternateContentSources/helpers.js
- test/fixtures/models/katello_alternate_content_sources.yml
- webpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.js
- app/services/katello/pulp3/smart_proxy_mirror_repository.rb
- app/services/katello/pulp3/api/apt.rb
- test/models/katello/alternate_content_source_test.rb
- test/actions/pulp3/orchestration/alternate_content_source_update_test.rb
- test/actions/pulp3/orchestration/alternate_content_source_refresh_test.rb
Contributor
Author
|
alright should hopefully be good to go now |
ianballou
reviewed
Apr 20, 2026
Member
There was a problem hiding this comment.
I found one issue, but otherwise this is working well! I think fixing it is optional for now, but should be taken care of down the road, ideally before this release gets out. Stabilization is in a couple of weeks. Let me know if you'd prefer to do it now or later. If later, then I'll finalize my review.
I've tested the feature via API and UI, covering the general CRUD operations that users would run through, and some corner cases around simplified ACSs and repo deletion.
Automated testing report for the curious: https://gist.github.com/ianballou/6171febdbb3f285a2a956696a2a5df80
I also hand-tested the feature via the UI.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/actions/pulp3/orchestration/alternate_content_source_create_test.rb (1)
63-70: Optional: add DEB PRN assertions for parity with yum/file tests.
test_yum_createandtest_file_createverify bothremote_prnandalternate_content_source_prnare present and match the expectedprn:<type>:<uuid>shape. Adding the DEB equivalents would make this test equally effective at catching PRN regressions in the new Pulp v3 DEB path.♻️ Proposed assertions
def test_deb_create ::Katello::Pulp3::AlternateContentSource.any_instance.stubs(:generate_backend_object_name).returns(`@deb_acs.name`) smart_proxy_acs = ::Katello::SmartProxyAlternateContentSource.create(alternate_content_source_id: `@deb_acs.id`, smart_proxy_id: `@primary.id`) ForemanTasks.sync_task(::Actions::Pulp3::Orchestration::AlternateContentSource::Create, smart_proxy_acs) assert_equal 1, `@deb_acs.smart_proxy_alternate_content_sources.count` assert `@deb_acs.smart_proxy_alternate_content_sources.first.remote_href.start_with`?('/pulp/api/v3/remotes/deb/apt/') assert `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_href.start_with`?('/pulp/api/v3/acs/deb/deb/') + assert_not_nil `@deb_acs.smart_proxy_alternate_content_sources.first.remote_prn` + assert_match(/^prn:deb\.aptremote:[0-9a-f\-]+$/, `@deb_acs.smart_proxy_alternate_content_sources.first.remote_prn`) + assert_not_nil `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_prn` + assert_match(/^prn:deb\.aptalternatecontentsource:[0-9a-f\-]+$/, `@deb_acs.smart_proxy_alternate_content_sources.first.alternate_content_source_prn`) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/orchestration/alternate_content_source_create_test.rb` around lines 63 - 70, Update test_deb_create to also assert the DEB PRNs like the yum/file tests: after creating smart_proxy_acs and syncing, fetch the created record via `@deb_acs.smart_proxy_alternate_content_sources.first` and add assertions that its remote_prn and alternate_content_source_prn are present and match the expected "prn:<type>:<uuid>" shape (use a regex similar to the other tests, e.g. /^prn:[a-z]+:[0-9a-f-]+$/), referencing the test method test_deb_create, the `@deb_acs` fixture and smart_proxy_alternate_content_sources to locate where to add the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/actions/pulp3/orchestration/alternate_content_source_create_test.rb`:
- Around line 63-70: Update test_deb_create to also assert the DEB PRNs like the
yum/file tests: after creating smart_proxy_acs and syncing, fetch the created
record via `@deb_acs.smart_proxy_alternate_content_sources.first` and add
assertions that its remote_prn and alternate_content_source_prn are present and
match the expected "prn:<type>:<uuid>" shape (use a regex similar to the other
tests, e.g. /^prn:[a-z]+:[0-9a-f-]+$/), referencing the test method
test_deb_create, the `@deb_acs` fixture and smart_proxy_alternate_content_sources
to locate where to add the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 204b66eb-f524-4250-980c-020747e9c702
📒 Files selected for processing (32)
app/controllers/katello/api/v2/alternate_content_sources_controller.rbapp/models/katello/alternate_content_source.rbapp/services/katello/pulp3/alternate_content_source.rbapp/services/katello/pulp3/api/apt.rbapp/services/katello/pulp3/smart_proxy_mirror_repository.rbapp/views/katello/api/v2/alternate_content_sources/base.json.rabldb/migrate/20251110000001_add_deb_fields_to_acs.rbtest/actions/pulp3/orchestration/alternate_content_source_create_test.rbtest/actions/pulp3/orchestration/alternate_content_source_delete_test.rbtest/actions/pulp3/orchestration/alternate_content_source_refresh_test.rbtest/actions/pulp3/orchestration/alternate_content_source_update_test.rbtest/actions/pulp3/orchestration/deb_sync_test.rbtest/controllers/api/v2/alternate_content_sources_controller_test.rbtest/fixtures/models/katello_alternate_content_sources.ymltest/fixtures/models/katello_repositories.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.ymltest/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.ymltest/fixtures/vcr_cassettes/actions/pulp3/apt_sync/sync_with_deb_acs.ymltest/models/katello/alternate_content_source_test.rbwebpack/scenes/AlternateContentSources/Create/ACSCreateWizard.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.jswebpack/scenes/AlternateContentSources/Create/Steps/ACSReview.jswebpack/scenes/AlternateContentSources/Create/Steps/AcsUrlPaths.jswebpack/scenes/AlternateContentSources/Create/Steps/SelectSource.jswebpack/scenes/AlternateContentSources/Create/__tests__/acsCreateDeb.test.jswebpack/scenes/AlternateContentSources/Details/ACSExpandableDetails.jswebpack/scenes/AlternateContentSources/Details/EditModals/ACSEditURLPaths.jswebpack/scenes/AlternateContentSources/helpers.js
✅ Files skipped from review due to trivial changes (10)
- test/fixtures/models/katello_repositories.yml
- app/views/katello/api/v2/alternate_content_sources/base.json.rabl
- db/migrate/20251110000001_add_deb_fields_to_acs.rb
- test/fixtures/models/katello_alternate_content_sources.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/http_proxy_url_update_deb_acs.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_delete/deb_delete.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh_updates_remote.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_create/deb_create.yml
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_refresh/deb_refresh.yml
- test/models/katello/alternate_content_source_test.rb
🚧 Files skipped from review as they are similar to previous changes (10)
- webpack/scenes/AlternateContentSources/Create/Steps/SelectSource.js
- webpack/scenes/AlternateContentSources/Create/ACSCreateWizard.js
- app/services/katello/pulp3/smart_proxy_mirror_repository.rb
- webpack/scenes/AlternateContentSources/Create/Steps/ACSCreateFinish.js
- test/actions/pulp3/orchestration/alternate_content_source_update_test.rb
- test/actions/pulp3/orchestration/alternate_content_source_refresh_test.rb
- webpack/scenes/AlternateContentSources/Create/tests/acsCreateDeb.test.js
- test/fixtures/vcr_cassettes/actions/pulp3/alternate_content_source_update/deb_update.yml
- app/controllers/katello/api/v2/alternate_content_sources_controller.rb
- app/models/katello/alternate_content_source.rb
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
6 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?
distributionsparameter. So it has been added to the creation process.componentsandarchitecturesfields have been included to match sync.subpathsparameter and is not used by debian ACS.What are the testing steps for this pull request?
Summary by Sourcery
Add full Debian content support for Alternate Content Sources by extending the database schema, model validations, API, UI wizard, and Pulp3 orchestration workflows, accompanied by extensive test coverage.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Chores
Tests