fixes #7890 Move Distributions from Elasticsearch to the Database #5362

johnpmitsch · 2015-07-20T15:52:17Z

Adds distribution values from pulp as attributes to Repository. They are added to the UI and users are now able to search on these values using scoped search

johnpmitsch · 2015-07-20T15:55:16Z

before testing: rake db:migrate && rake katello:upgrades:2.4:import_distributions

ehelms · 2015-07-20T16:03:37Z

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

@@ -1,38 +0,0 @@
-module Katello
-  class Api::V2::DistributionsController < Api::V2::ApiController


If we are removing this, we need to also remove the routes related to it, further, we need to ask ourselves if this is breaking our API contract and if we need to rev our versioning in light of.

Would it be a better idea to leave it in so the API functions as it did before? and that would mean leaving in the Distribution model and Glue::ElasticSearch::Distribution as well, right?

Since we're completely getting rid of an object, I'm fairly okay with this

Not sure I follow exactly, you are saying because we are removing an object from conceptual existence, you are OK treating this API removal as not a breaking change?

Pretty much. I'm sure others would disagree, but really this api doesn't even make sense. the index call requires a repository_id and returns a list, even though there will ever be one.

We could maintain this api (minus the searching) with these attributes: :version, :arch, :family, :variant simply by mapping the repo object i guess. That would be fairly simple. Maintaining the full api (minus search syntax compatibility, which i guess we've determined is okay to change) would basically dictate how the model is done (and not done the way we discussed).

I suppose we could throw a deprecation warning into 2.3?

That sounds like a plan!

Just so I understand, @jlsherrill when you say mapping the repo object, do you mean adding the distribution attributes to the repository api (which is done here) or making a child object "distribution" of the repo object when showing a repository in the api?

I was suggesting we could maintain the existing controller, but instead of rendering distribution objects, we render repo objects with the distribution attributes. But eric is now suggesting we just deprecate the distribution api in the 2.3 release (which is not out yet), and get rid of it in this PR

johnpmitsch · 2015-07-20T17:21:15Z

[test]

ehelms · 2015-07-20T17:33:48Z

app/views/katello/api/v2/repositories/show.json.rabl

@@ -15,6 +15,11 @@ attributes :gpg_key_id
 attributes :content_id, :content_view_version_id, :library_instance_id
 attributes :product_type
 attributes :promoted? => :promoted
+attributes :distribution_version
+attributes :distribution_arch
+attributes :distribution_bootable? => :distribution_bootable


This is what started my question around this function, as it seems like you should just rely on the database attribute instead of mapping a function to this attribute?

johnpmitsch · 2015-08-18T21:45:50Z

[test]

ehelms · 2015-08-24T14:28:38Z

@johnpmitsch looks like you need a rebase here

johnpmitsch · 2015-08-24T14:41:03Z

@ehelms Thanks, just rebased

jlsherrill · 2015-08-25T14:14:22Z

app/models/katello/content_view.rb

-      conflicts.select { |conflict| conflict[:distributions].length > 1 }
+      repos =  self.repositories_to_publish.where("distribution_arch IS NOT NULL OR distribution_version IS NOT NULL")
+      repos.group_by { |repo| [repo.distribution_arch, repo.distribution_version] }
+           .select { |_, value| value.length > 1 }.values.flatten


This should only include bootable distros right?

(it doesn't look like it does)

ya it was including all distros, I just changed it, thanks!

maybe i'm misreading this, but i don't see where that was changed?

johnpmitsch · 2015-08-26T17:23:44Z

app/models/katello/content_view.rb

-      conflicts.select { |conflict| conflict[:distributions].length > 1 }
+      repos =  self.repositories_to_publish.where("distribution_arch IS NOT NULL OR distribution_version IS NOT NULL")
+      repos = repos.select { |repo| repo.distribution_bootable? }
+      repos.group_by { |repo| [repo.distribution_arch, repo.distribution_version] }


@jlsherrill ok that should do it

jlsherrill · 2015-08-27T17:10:37Z

ACK from me, ehelms?

ehelms · 2015-08-27T18:58:35Z

👍

jlsherrill · 2015-08-27T20:00:08Z

Merging, thanks @johnpmitsch

fixes #7890 Move Distributions from Elasticsearch to the Database

theforeman-bot added Needs testing Not yet reviewed labels Jul 20, 2015

ehelms reviewed Jul 20, 2015
View reviewed changes

johnpmitsch force-pushed the distributions branch 8 times, most recently from 169a44b to e430466 Compare July 27, 2015 15:33

johnpmitsch force-pushed the distributions branch from e430466 to 9b98ff8 Compare July 30, 2015 13:24

ehelms added Waiting on contributor Needs re-review and removed Not yet reviewed labels Jul 31, 2015

johnpmitsch force-pushed the distributions branch from 9b98ff8 to 6db574a Compare August 6, 2015 15:43

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Aug 6, 2015

johnpmitsch force-pushed the distributions branch 4 times, most recently from f09c907 to 00b11b0 Compare August 10, 2015 21:06

johnpmitsch force-pushed the distributions branch 9 times, most recently from 0ffbf8b to b7c6286 Compare August 18, 2015 20:24

johnpmitsch force-pushed the distributions branch 3 times, most recently from 651dba4 to 9ce87dc Compare August 19, 2015 20:38

ehelms added the Waiting on contributor label Aug 24, 2015

johnpmitsch force-pushed the distributions branch from 9ce87dc to 769c87e Compare August 24, 2015 14:40

theforeman-bot added Needs testing Needs re-review and removed Waiting on contributor labels Aug 24, 2015

jlsherrill reviewed Aug 25, 2015
View reviewed changes

johnpmitsch force-pushed the distributions branch from 769c87e to b4825e9 Compare August 25, 2015 15:24

fixes Katello#7890 Move Distributions from Elasticsearch to the Database

e1e6292

johnpmitsch force-pushed the distributions branch from b4825e9 to e1e6292 Compare August 26, 2015 16:43

johnpmitsch reviewed Aug 26, 2015
View reviewed changes

jlsherrill added a commit that referenced this pull request Aug 27, 2015

Merge pull request #5362 from johnpmitsch/distributions

6af70c2

fixes #7890 Move Distributions from Elasticsearch to the Database

jlsherrill merged commit 6af70c2 into Katello:master Aug 27, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #7890 Move Distributions from Elasticsearch to the Database #5362

fixes #7890 Move Distributions from Elasticsearch to the Database #5362

johnpmitsch commented Jul 20, 2015

johnpmitsch commented Jul 20, 2015

ehelms Jul 20, 2015

johnpmitsch Jul 20, 2015

jlsherrill Jul 21, 2015

ehelms Jul 21, 2015

jlsherrill Jul 21, 2015

ehelms Jul 21, 2015

jlsherrill Jul 21, 2015

johnpmitsch Jul 21, 2015

jlsherrill Jul 21, 2015

johnpmitsch commented Jul 20, 2015

ehelms Jul 20, 2015

johnpmitsch commented Aug 18, 2015

ehelms commented Aug 24, 2015

johnpmitsch commented Aug 24, 2015

jlsherrill Aug 25, 2015

jlsherrill Aug 25, 2015

johnpmitsch Aug 25, 2015

jlsherrill Aug 26, 2015

johnpmitsch Aug 26, 2015

johnpmitsch Aug 26, 2015

jlsherrill commented Aug 27, 2015

ehelms commented Aug 27, 2015

jlsherrill commented Aug 27, 2015

		@@ -1,38 +0,0 @@
		module Katello
		class Api::V2::DistributionsController < Api::V2::ApiController

fixes #7890 Move Distributions from Elasticsearch to the Database #5362

fixes #7890 Move Distributions from Elasticsearch to the Database #5362

Conversation

johnpmitsch commented Jul 20, 2015

johnpmitsch commented Jul 20, 2015

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

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

johnpmitsch commented Jul 20, 2015

Choose a reason for hiding this comment

johnpmitsch commented Aug 18, 2015

ehelms commented Aug 24, 2015

johnpmitsch commented Aug 24, 2015

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

Choose a reason for hiding this comment

jlsherrill commented Aug 27, 2015

ehelms commented Aug 27, 2015

jlsherrill commented Aug 27, 2015