Fixes #20057 - fix duplicate akey repo sets #6973

jlsherrill · 2017-09-25T19:50:25Z

this fixes duplicate repository sets from being returned
when listing them for an activation key. In addition this improves performance
by using a single api call for all product content in the org,
as well as using a presenter to only fetch the activation key overrides
once, instead of once per product content.

In addition this updates the redhat repos page to use this new
content fetching method to speed up that page as well.

theforeman-bot · 2017-09-25T19:50:29Z

Issues: #20057

theforeman-bot · 2017-09-25T19:50:29Z

There were the following issues with the commit message:

commit message for d719c7d9018d5e8e88cfa22d8dbfa9df7da26f9a is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

houndci-bot · 2017-09-25T19:50:40Z

app/helpers/katello/providers_helper.rb

      provider.products.each do |product|
-        product.displayable_product_contents.each do |prod_content|
+        selected_contents = product_content.select { |product_content| product_content.product_id == product.id }.select(&:displayable?)


Shadowing outer local variable - product_content.

parthaa · 2017-09-29T02:05:26Z

app/models/katello/glue/candlepin/owner.rb

@@ -38,6 +38,32 @@ def content_access_mode
        self.owner_details['contentAccessMode']
      end

+      def enabled_product_content(for_repositories)
+        return [] if for_repositories && for_repositories.empty?


for_repositories.blank?

parthaa · 2017-09-29T02:12:39Z

app/models/katello/glue/candlepin/owner.rb

+      def enabled_product_content(for_repositories)
+        return [] if for_repositories && for_repositories.empty?
+        repositories = for_repositories.select([:product_id, :content_id]).to_a
+        content_ids = repositories.map(&:content_id)


If you are just using content_id from here then why do you need [:product_id, :content_id]

parthaa · 2017-09-29T02:16:17Z

app/models/katello/glue/candlepin/owner.rb

+      end
+
+      def filtered_product_content
+        cp_products = Katello::Resources::Candlepin::Product.all(Organization.first.label, [:id, :productContent])


I dont remember exactly how this works. Can you enlighten me on why you are getting all products from Organization.first as opposed to current org?

that should be self ;) /me thinks i copy and pasted from my console

jlsherrill · 2017-09-29T15:52:34Z

@parthaa updated

parthaa · 2017-10-03T06:23:50Z

app/lib/katello/resources/candlepin.rb

@@ -399,6 +399,10 @@ def import(organization_name, path_to_file, options)
            self.post(path, {:import => File.new(path_to_file, 'rb')}, self.default_headers.except('content-type'))
          end

+          def product_content(organization_name)
+            Product.all(organization_name, [:id, :productContent])


This is no chance this would be confused with '::Katello::Product' right ?

I don't think so, we do things in similar places in this file

parthaa · 2017-10-03T06:32:16Z

Code looks good to me. But the "show all " in the repository sets page seems to show me non yum repos also (like I saw paths pointing to ostree and docker). In the master version the docker ones were getting ignored. And I don't know if ostree stuff can be overridden. So might not not want to include that,

parthaa · 2017-10-03T06:33:20Z

app/models/katello/activation_key.rb

-      end
-      content.uniq
+      considered_products = content_access_mode_all ? all_products : self.products
+      repositories = Katello::Repository.where(:product_id => considered_products)


Might need to limit this to type yum.

fixed by using .subscribable

parthaa · 2017-10-03T06:44:52Z

I definitely notice the perf improvements definitely and no dupes. Nice work. Address the above comment and should be good to go.

parthaa · 2017-10-03T21:13:58Z

app/models/katello/glue/candlepin/owner.rb

+        to_return = []
+
+        cp_products.each do |product_hash|
+          product = ::Katello::Product.find_by(:cp_id => product_hash['id'])


cp_id is not org safe!

- product = ::Katello::Product.find_by(:cp_id => product_hash['id']) + product = ::Katello::Product.find_by(:organization_id => self.id, :cp_id => product_hash['id'])

parthaa · 2017-10-03T21:23:24Z

Not to overcomplicate this PR but can you check if the content hosts could use this logic for subs and repo sets... My guess is they are suffering from the same ailments

parthaa · 2017-10-03T21:40:15Z

Rh Repos page

Before PR
 Completed 200 OK in 26777ms (Views: 26283.9ms | ActiveRecord: 85.3ms)

After PR
 Completed 200 OK in 8058ms (Views: 6534.3ms | ActiveRecord: 1156.0ms)

jlsherrill · 2017-10-04T14:42:16Z

Updated to fix multi-org issue, added to content host repo sets listing, and sped it up even more by using more specific includes on the candlepin request.

theforeman-bot · 2017-10-04T20:53:52Z

There were the following issues with the commit message:

2f345b3329c6622fdf4bebe2fff2c1382893657d must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

jlsherrill · 2017-10-04T20:54:29Z

Updated to move to a service as much as i could. let me know if you like it better

parthaa · 2017-10-04T21:05:00Z

app/controllers/katello/api/v2/activation_keys_controller.rb

+      content = pcf.product_content
+
+      overrides = @activation_key.content_overrides
+      content.map! { |pc| ProductContentPresenter.new(pc, overrides) }


I like the refactoring but I think you could DRY it up further by adding overrides logic to the Finder and make him return ProductContentPresenter... That would mean we can use the same logic for both AK and CH

theforeman-bot · 2017-10-05T16:10:59Z

There were the following issues with the commit message:

310cf1e3d5d39ed823bad3a5aee85bd75d0ff87b must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

jlsherrill · 2017-10-05T16:11:27Z

@parthaa updated!

parthaa · 2017-10-05T20:45:11Z

APJ

this fixes duplicate repository sets from being returned when listing them for an activation key. In addition this improves performance by using a single api call for all product content in the org, as well as using a presenter to only fetch the activation key overrides once, instead of once per product content. In addition this updates the redhat repos page to use this new content fetching method to speed up that page as well. Move product content to a service

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Sep 25, 2017

houndci-bot reviewed Sep 25, 2017

View reviewed changes

jlsherrill force-pushed the 20057 branch from d719c7d to 62e3726 Compare September 25, 2017 19:54

theforeman-bot added Needs testing and removed Waiting on contributor labels Sep 25, 2017

jlsherrill force-pushed the 20057 branch from 62e3726 to f6cadff Compare September 26, 2017 18:55

parthaa changed the title ~~Fixes #20057 - fix dupliate akey repo sets~~ Fixes #20057 - fix duplicate akey repo sets Sep 28, 2017

parthaa self-assigned this Sep 28, 2017

parthaa reviewed Sep 29, 2017

View reviewed changes

jlsherrill force-pushed the 20057 branch from f6cadff to fb775c6 Compare September 29, 2017 15:50

parthaa reviewed Oct 3, 2017

View reviewed changes

jlsherrill force-pushed the 20057 branch from fb775c6 to 15fe0ca Compare October 3, 2017 20:10

parthaa reviewed Oct 3, 2017

View reviewed changes

jlsherrill force-pushed the 20057 branch 2 times, most recently from 1b3519d to 5330a4d Compare October 4, 2017 14:41

theforeman-bot added the Waiting on contributor label Oct 4, 2017

parthaa reviewed Oct 4, 2017

View reviewed changes

jlsherrill force-pushed the 20057 branch from 2f345b3 to 310cf1e Compare October 5, 2017 16:10

theforeman-bot added Needs testing Waiting on contributor and removed Waiting on contributor labels Oct 5, 2017

jlsherrill force-pushed the 20057 branch from 310cf1e to 18f6e6b Compare October 6, 2017 17:11

theforeman-bot added Needs testing and removed Waiting on contributor labels Oct 6, 2017

jlsherrill merged commit 2c8fc1e into Katello:master Oct 6, 2017

jlsherrill deleted the 20057 branch October 6, 2017 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #20057 - fix duplicate akey repo sets #6973

Fixes #20057 - fix duplicate akey repo sets #6973

jlsherrill commented Sep 25, 2017

theforeman-bot commented Sep 25, 2017

theforeman-bot commented Sep 25, 2017

houndci-bot Sep 25, 2017

parthaa Sep 29, 2017

parthaa Sep 29, 2017

parthaa Sep 29, 2017

jlsherrill Sep 29, 2017

jlsherrill commented Sep 29, 2017

parthaa Oct 3, 2017

jlsherrill Oct 3, 2017

parthaa commented Oct 3, 2017 •

edited

parthaa Oct 3, 2017

jlsherrill Oct 3, 2017

parthaa commented Oct 3, 2017

parthaa Oct 3, 2017

parthaa Oct 3, 2017

parthaa commented Oct 3, 2017

parthaa commented Oct 3, 2017

jlsherrill commented Oct 4, 2017

theforeman-bot commented Oct 4, 2017

jlsherrill commented Oct 4, 2017

parthaa Oct 4, 2017

theforeman-bot commented Oct 5, 2017

jlsherrill commented Oct 5, 2017

parthaa commented Oct 5, 2017

Fixes #20057 - fix duplicate akey repo sets #6973

Fixes #20057 - fix duplicate akey repo sets #6973

Conversation

jlsherrill commented Sep 25, 2017

theforeman-bot commented Sep 25, 2017

theforeman-bot commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlsherrill commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa commented Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa commented Oct 3, 2017

parthaa commented Oct 3, 2017

jlsherrill commented Oct 4, 2017

theforeman-bot commented Oct 4, 2017

jlsherrill commented Oct 4, 2017

Choose a reason for hiding this comment

theforeman-bot commented Oct 5, 2017

jlsherrill commented Oct 5, 2017

parthaa commented Oct 5, 2017

parthaa commented Oct 3, 2017 •

edited