Drop Pulp 2 Smart Proxy status code by ekohl · Pull Request #10721 · Katello/katello

ekohl · 2023-09-06T09:27:30Z

What are the changes introduced in this pull request?

It drops the Pulp 2 Smart Proxy status extensions.

Considerations taken when implementing this change?

These were likely not triggered anymore because the Pulp 2 features were never present.

What are the testing steps for this pull request?

Go to the Smart Proxies overview and for every Smart Proxy it should still render well. I forgot if there's a Pulp 3 tab today, but if there is, it should still be present after.

ianballou · 2024-01-24T20:08:25Z

Guessing we missed this since it didn't show up in triage via redmine, mind adding one so we can get it on our job board?

ianballou

engine.rb still has ::SmartProxiesController.include Katello::Concerns::SmartProxiesControllerExtensions in it, removing that should fix the tests.

ekohl · 2024-06-26T12:29:35Z

Rebased and removed require statements.

ekohl · 2024-06-26T13:38:20Z

I'm not 100% sure if this also needs to be reflected in any templates.

ianballou

Looks like it's breaking the smart proxy details pages:

2024-07-16T20:18:30 [F|app|3fb86828]   
 3fb86828 | ActionView::Template::Error (undefined method `map' for nil:NilClass):
 3fb86828 |     1: <%= render :partial => 'disk_usage', :collection => disks(@storage), :as => :disk %>
 3fb86828 |   
 3fb86828 | /home/vagrant/katello/app/helpers/katello/concerns/smart_proxy_helper_extensions.rb:5:in `disks'
 3fb86828 | /home/vagrant/katello/app/views/smart_proxies/pulp_storage.html.erb:1
 3fb86828 | app/controllers/concerns/foreman/controller/timezone.rb:10:in `set_timezone'
 3fb86828 | app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
 3fb86828 | app/controllers/concerns/foreman/controller/topbar_sweeper.rb:12:in `set_topbar_sweeper_controller'
 3fb86828 | /home/vagrant/katello/lib/katello/middleware/organization_created_enforcer.rb:18:in `call'
 3fb86828 | /home/vagrant/katello/lib/katello/middleware/event_daemon.rb:10:in `call'
 3fb86828 | lib/foreman/middleware/libvirt_connection_cleaner.rb:9:in `call'
 3fb86828 | lib/foreman/middleware/telemetry.rb:10:in `call'
 3fb86828 | lib/foreman/middleware/logging_context_session.rb:22:in `call'
 3fb86828 | lib/foreman/middleware/logging_context_request.rb:11:in `call'
 3fb86828 | /home/vagrant/katello/lib/katello/prevent_json_parsing.rb:12:in `call'
2024-07-16T20:18:32 [I|app|3c441f02] Processing by Katello::Api::V2::CapsuleContentController#sync_status as JSON

ianballou · 2024-07-16T21:09:53Z

I think the Pulp and Pulp Node bits can also be removed from: https://github.com/katello/katello/blob/master/lib/katello/plugin.rb#L313

ekohl · 2024-07-18T12:52:09Z

I think this should be done in factories as traits, but I considered it out of scope here.

ekohl · 2024-07-18T12:59:12Z

I thought about optimizing this to:

unscoped.select { |p| p.pulp_mirror? }.joins(:capsule_lifecycle_environments). where(katello_capsule_lifecycle_environments: { lifecycle_environment_id: environment.id }).distinct

This would be fine, I like avoiding excess queries where possible.

ekohl · 2024-07-18T12:59:39Z

Suggested change

def self.with_environment(environment, include_default = false)

def self.with_environment(environment)

This should be fine, I don't see any uses of include_default = false with with_environment().

ianballou

Something is still broken on the smart proxies page, looks like storage is still ending up nil.

 48a2c081 | ActionView::Template::Error (undefined method `map' for nil:NilClass):
 48a2c081 |     1: <%= render :partial => 'disk_usage', :collection => disks(@storage), :as => :disk %>
 48a2c081 |   
 48a2c081 | /home/vagrant/katello/app/helpers/katello/concerns/smart_proxy_helper_extensions.rb:5:in `disks'
 48a2c081 | /home/vagrant/katello/app/views/smart_proxies/pulp_storage.html.erb:1
 48a2c081 | app/controllers/concerns/foreman/controller/timezone.rb:10:in `set_timezone'
 48a2c081 | app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
 48a2c081 | app/controllers/concerns/foreman/controller/topbar_sweeper.rb:12:in `set_topbar_sweeper_controller'
 48a2c081 | /home/vagrant/katello/lib/katello/middleware/organization_created_enforcer.rb:18:in `call'
 48a2c081 | /home/vagrant/katello/lib/katello/middleware/event_daemon.rb:10:in `call'
 48a2c081 | lib/foreman/middleware/libvirt_connection_cleaner.rb:9:in `call'
 48a2c081 | lib/foreman/middleware/telemetry.rb:10:in `call'
 48a2c081 | lib/foreman/middleware/logging_context_session.rb:22:in `call'
 48a2c081 | lib/foreman/middleware/logging_context_request.rb:11:in `call'
 48a2c081 | /home/vagrant/katello/lib/katello/prevent_json_parsing.rb:12:in `call'

ianballou

I haven't figured out why, but this PR is stopping find_resource from working. I have to manually add find_resource to controller methods for them to work in both Foreman and Katello. For example, there are nil errors in smart_proxies_controller#modules_card for @smart_proxy.statuses

ianballou · 2025-05-05T18:28:42Z

This line causes init to fail.

ekohl · 2025-05-06T09:10:03Z

@ianballou would you be willing to take this over? I don't have the time to focus on this, but I still think cleaning up unused code is something that should be done.

ianballou · 2025-05-06T13:27:28Z

@ianballou would you be willing to take this over? I don't have the time to focus on this, but I still think cleaning up unused code is something that should be done.

Sure, I can take it over 👍

The PULP_FEATURE and PULP_NODE_FEATURE features were for Pulp 2. This is never present in a current Katello installation and can be dropped. It also replaces the deprecated default_capsule code, which is replaced by pulp_primary. In the template the code to handle primaries is disabled because it's only shown for mirrors. A mirror is by definition not a primary, so it can never trigger.

ekohl · 2025-08-02T00:02:39Z

          pulp_primary || fail(_("Could not find a smart proxy with pulp feature."))
        end

+        def self.default_capsule


Where is this used?

Funny enough, the foreman_rh_cloud plugin uses it. It should switch to using pulp_primary ideally.

theforeman-bot added Waiting on contributor Not yet reviewed labels Sep 6, 2023

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch from 0d9ad06 to bd0f12f Compare January 23, 2024 11:22

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch from bd0f12f to 8559d28 Compare April 26, 2024 09:01

ekohl marked this pull request as ready for review April 26, 2024 09:01

pr-processor Bot removed the Waiting on contributor label Apr 26, 2024

ianballou requested changes May 8, 2024

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Not yet reviewed labels May 8, 2024

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch from 8559d28 to 562c66d Compare May 8, 2024 20:37

pr-processor Bot added Needs re-review and removed Waiting on contributor labels May 8, 2024

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch from 562c66d to d58b925 Compare June 26, 2024 12:28

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch from d58b925 to 2d2f2f5 Compare June 26, 2024 12:59

ekohl commented Jun 26, 2024

View reviewed changes

ianballou self-requested a review June 27, 2024 17:58

ianballou reviewed Jul 16, 2024

View reviewed changes

Comment thread app/controllers/katello/concerns/smart_proxies_controller_extensions.rb Outdated

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch 2 times, most recently from 749d654 to ab383c3 Compare July 18, 2024 12:49

ekohl commented Jul 18, 2024

View reviewed changes

Comment thread app/assets/javascripts/katello/containers/container.js Outdated

ekohl commented Jul 18, 2024

View reviewed changes

ianballou reviewed Jul 26, 2024

View reviewed changes

ekohl force-pushed the drop-pulp-2-smart-proxy-code branch 2 times, most recently from f1bc1c7 to 6990829 Compare February 18, 2025 18:43

ianballou reviewed May 5, 2025

View reviewed changes

Comment thread app/models/katello/concerns/smart_proxy_extensions.rb Outdated

ianballou May 5, 2025

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line causes init to fail.

pr-processor Bot removed the Needs re-review label May 5, 2025

ekohl and others added 2 commits August 1, 2025 18:58

Refs #37388 - remove use of find_resource that broke callback chain

dab107b

ianballou force-pushed the drop-pulp-2-smart-proxy-code branch from 6990829 to dab107b Compare August 1, 2025 21:52

ekohl commented Aug 2, 2025

View reviewed changes

	def self.with_environment(environment, include_default = false)
	def self.with_environment(environment)

Uh oh!

Conversation

ekohl commented Sep 6, 2023

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Uh oh!

ianballou commented Jan 24, 2024

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

ekohl commented Jun 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

ianballou commented Jul 16, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

ianballou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl commented May 6, 2025

Uh oh!

ianballou commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants