Fixes #38912 - Update deb content URL options via metadata generate

[quba42]

https://projects.theforeman.org/issues/38912

Summary by Sourcery

Update repository metadata generation to keep Debian content URLs in sync with deb-specific URL options and improve APT repository metadata handling.

Bug Fixes:

• Ensure Debian repository metadata generation triggers a Candlepin product content update when deb_content_url_options change so content URLs stay consistent.
• Prevent container push library repositories from planning metadata generation by reusing the root reference and early-returning correctly.
• Guarantee deterministic ordering of APT components and distributions returned from Pulp by sorting the unique lists.

Tests:

• Extend metadata generation tests to cover Debian repositories, validating deb_content_url_options behavior and whether Candlepin content updates are planned appropriately.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• In MetadataGenerate, the condition !repository.content.content_url.end_with?(repository.deb_content_url_options) assumes deb_content_url_options is always a non-empty string; consider guarding against nil/blank values to avoid potential NoMethodError and unnecessary ContentUpdate scheduling.
• The comparison between content_url and deb_content_url_options using end_with? is sensitive to exact string formatting (e.g., trailing slashes or query parameter ordering); if those can vary, consider normalizing/parsing the URL/query parameters instead of a raw suffix check to avoid spurious ContentUpdate actions.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In MetadataGenerate, the condition `!repository.content.content_url.end_with?(repository.deb_content_url_options)` assumes `deb_content_url_options` is always a non-empty string; consider guarding against nil/blank values to avoid potential NoMethodError and unnecessary ContentUpdate scheduling.
- The comparison between `content_url` and `deb_content_url_options` using `end_with?` is sensitive to exact string formatting (e.g., trailing slashes or query parameter ordering); if those can vary, consider normalizing/parsing the URL/query parameters instead of a raw suffix check to avoid spurious ContentUpdate actions.

## Individual Comments

### Comment 1
<location> `test/actions/katello/repository/metadata_generate_test.rb:98-107` </location>
<code_context>
+      refute_action_planned(action, candlepin_product_content_update_class)
+    end
+
+    it 'plans a deb metadata generate with changed deb_content_url_options' do
+
+      action = create_action(action_class)
+      action.stubs(:task).returns(success_task)
+      action.expects(:action_subject).with(deb_repo)
+      deb_repo.expects(:deb_content_url_options).returns("/?comp=main,contrib&rel=bookworm")
+
+      plan_action(action, deb_repo)
+
+      assert_equal(deb_repo.content.content_url, "/custom/Debian_12/Debian_12_amd64_main/?comp=main&rel=bookworm")
+      assert_action_planned_with(action, pulp_metadata_generate_class, deb_repo, SmartProxy.pulp_primary,
+            action_options)
+      assert_action_planned(action, candlepin_product_content_update_class)
+    end
+
</code_context>

<issue_to_address>
**suggestion (testing):** Strengthen the ContentUpdate test by asserting the planned action’s arguments

Right now this test only checks that a Candlepin ContentUpdate action is planned, but not that it’s called with the correct parameters (owner, label, content_url, etc.). To better validate the wiring from metadata generation to Candlepin and catch regressions if those attributes change, please assert the arguments passed to the planned action (for example with assert_action_planned_with or an equivalent helper you use elsewhere).

Suggested implementation:

```ruby
      assert_equal(deb_repo.content.content_url, "/custom/Debian_12/Debian_12_amd64_main/?comp=main&rel=bookworm")
      assert_action_planned_with(action, pulp_metadata_generate_class, deb_repo, SmartProxy.pulp_primary,
            action_options)

      # Ensure Candlepin ContentUpdate is planned with the expected arguments
      # (repository product + content are the typical inputs for updating Candlepin content)
      assert_action_planned_with(
        action,
        candlepin_product_content_update_class,
        deb_repo.product,
        deb_repo.content
      )
    end

```

To fully align this with the existing conventions in your test suite, please:
1) Verify the exact argument list used for `Actions::Candlepin::Product::ContentUpdate` in other tests (e.g. other references to `candlepin_product_content_update_class` or `ContentUpdate` actions) and adjust the `assert_action_planned_with` arguments accordingly (for example, if the action expects `owner`, `content_label`, or additional options, include them here in the same order).
2) If your `assert_action_planned_with` helper requires keyword-style or hash arguments (e.g. `assert_action_planned_with(action, klass, :product => ..., :content => ..., :owner => ...)`), update the call to match that signature.
3) If `deb_repo.product` or `deb_repo.content` are not the correct objects (for example, if you need `deb_repo.root.product` or `deb_repo.content_view_environment.content`), substitute the appropriate objects that are used elsewhere when planning `ContentUpdate` for the same repository type.
</issue_to_address>

### Comment 2
<location> `test/actions/katello/repository/metadata_generate_test.rb:79-80` </location>
<code_context>
       assert_action_planned_with(action, pulp_metadata_generate_class, yum_repo, SmartProxy.pulp_primary,
                                 yum_action_options)
     end
+
</code_context>

<issue_to_address>
**suggestion (testing):** Consider asserting that Candlepin ContentUpdate is not planned for non-Debian repositories

In the existing yum metadata tests, please also assert that no Candlepin ContentUpdate action is planned for non-Debian repos (e.g., add refute_action_planned(action, candlepin_product_content_update_class) here). This will better capture the intent of the new Debian-specific behavior and prevent regressions where ContentUpdate might be triggered for other repository types.

Suggested implementation:

```ruby
      assert_action_planned_with(action, pulp_metadata_generate_class, yum_repo, SmartProxy.pulp_primary,
                                yum_action_options)
      refute_action_planned(action, candlepin_product_content_update_class)
    end

```

- If there are other existing tests in this file that exercise yum (or other non-Debian) repositories and verify that `pulp_metadata_generate_class` is planned, you should similarly add:
  `refute_action_planned(action, candlepin_product_content_update_class)`
  in those examples to ensure Candlepin ContentUpdate is only planned for Debian repositories.
- No further changes should be needed if `refute_action_planned` is already available from the test helper mixins used in this test file.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[quba42]

This is also a candidate for backporting to Katello 4.19.

[m-bucher]

Test are green and changes look sane.
The VCR-cassete re-records are due to the way we handle communication to Pulp-API in Debian repositories.

[m-bucher]

Eventually we should evaluate, if we want a helper-method that plans Candlepin::Product::ContentUpdate with a given Katello::Repository-object. IMHO we always want to be Candlepin and Katello Content to be in-sync.
Maybe even modify the Action itself to only receive the repository-object.

But that is out-of-scope for this PR, but maybe worth a thought.

[ianballou]

I agree here, it doesn't look like we ever use the method without feeding it a repository. Since this needs to get into 4.19.0, I'm okay with it happening some other time.

[ianballou]

This is looking good to me, I did a quick smoke test with a deb repo.

[ianballou]

I agree here, it doesn't look like we ever use the method without feeding it a repository. Since this needs to get into 4.19.0, I'm okay with it happening some other time.

[ianballou]

I let y'all merge it in case you need a last-minute look.