Fixes #12405: move packages from system to host. #5656

waldenraines · 2015-12-08T14:50:34Z

http://projects.theforeman.org/issues/12405

Do not merge until this PR has an associated ACK'd hammer PR.

ehelms · 2015-12-14T14:51:16Z

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

-      @system = System.where(:uuid => params[:system_id]).first
-      fail HttpErrors::NotFound, _("Couldn't find system '%s'") % params[:system_id] if @system.nil?
+      @system = System.where(:host_id => params[:host_id]).first
+      fail HttpErrors::NotFound, _("Couldn't find system with host id '%s'") % params[:host_id] if @system.nil?


Should this be Couldn't find host ?

waldenraines · 2015-12-17T16:30:23Z

Associated hammer pr: Katello/hammer-cli-katello#340

jlsherrill · 2015-12-18T22:19:40Z

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

+    def find_host
+      @host = ::Host::Managed.find(params[:host_id])
+      fail HttpErrors::NotFound, _("Couldn't find host with host id '%s'") % params[:host_id] if @host.nil?
+      @host
    end


I would add a check here to verify the host has a content facet and a uuid in that content facet (implying its registered via pulp)

waldenraines · 2015-12-19T00:35:08Z

@ehelms @jlsherrill updated.

jlsherrill · 2015-12-20T16:16:05Z

Couple of comments:

a) you might also add nvra as searchable
b) The package list isn't sorted at all, should likely default to being sorted by name
c) The package list for a host is paged, but that page only has a filter, we probably want to change it to be a full search. Maybe convert this to details nutupane?
d) The package list table seems 'off':

e) The 'remove' button beside each package is missing, i'm pretty sure this is an existing problem but if its a simple fix, would be great to fix it here (especially if converting to details-nutupane)

waldenraines · 2016-01-06T16:17:37Z

@jlsherrill updated.

jlsherrill · 2016-01-09T18:47:27Z

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

-      fail HttpErrors::NotFound, _("Couldn't find system '%s'") % params[:system_id] if @system.nil?
-      @system
+    def find_host
+      @host = ::Host::Managed.find(params[:host_id])


As mentioned in your other PR, this doesn't properly check authorization

This looks like its still not properly authorizing

jlsherrill · 2016-01-11T22:02:49Z

Also discovered that we need something like this:

    resource_description do
      api_version 'v2'
      api_base_url "/api"
    end

in any controller that is under /api/hosts (and not under /katello/api)

waldenraines · 2016-01-12T15:39:23Z

Also discovered that we need something like this:
resource_description do
  api_version 'v2'
  api_base_url "/api"
end
in any controller that is under /api/hosts (and not under /katello/api)

See https://github.com/Katello/katello/pull/5656/files#diff-c488bbb43ed74b3c1446892671b7afe8R7

waldenraines · 2016-01-12T16:58:42Z

@jlsherrill updated.

jlsherrill · 2016-01-13T14:08:21Z

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

+      query = @host.installed_packages
+      collection = scoped_search(query.uniq, :name, :asc, :resource_class => ::Katello::InstalledPackage)
+      respond_for_index(:collection => collection)
+    end


would you be up for adding auto-complete support. Its really easy! :)

jlsherrill · 2016-01-13T14:34:19Z

Everything else looks great and works fine. After those 3 fixes I think its good to go.

http://projects.theforeman.org/issues/12405

waldenraines · 2016-01-15T14:14:29Z

@jlsherrill updated.

jlsherrill · 2016-01-18T17:57:34Z

ACK

Fixes #12405: move packages from system to host.

theforeman-bot added Needs testing Not yet reviewed labels Dec 8, 2015

waldenraines mentioned this pull request Dec 8, 2015

Fixes #12405: move packages to host. #5575

Closed

waldenraines force-pushed the 12405 branch 2 times, most recently from a675869 to ec2a068 Compare December 9, 2015 14:27

waldenraines added the Blocked by another PR label Dec 9, 2015

waldenraines force-pushed the 12405 branch 3 times, most recently from 7f0ccda to bea37f7 Compare December 10, 2015 16:38

ehelms reviewed Dec 14, 2015
View reviewed changes

ehelms added Waiting on contributor and removed Not yet reviewed labels Dec 14, 2015

waldenraines force-pushed the 12405 branch from bea37f7 to 08449d5 Compare December 17, 2015 16:15

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

waldenraines force-pushed the 12405 branch 2 times, most recently from 4077c44 to 62075b8 Compare December 17, 2015 20:56

jlsherrill reviewed Dec 18, 2015
View reviewed changes

theforeman-bot added the Waiting on contributor label Dec 19, 2015

waldenraines force-pushed the 12405 branch from f0c2a57 to e9ac318 Compare December 19, 2015 00:28

waldenraines force-pushed the 12405 branch from e9ac318 to e993ce6 Compare December 19, 2015 00:34

waldenraines force-pushed the 12405 branch 4 times, most recently from 825e8e4 to ed11696 Compare January 6, 2016 16:15

jlsherrill reviewed Jan 9, 2016
View reviewed changes

waldenraines force-pushed the 12405 branch 3 times, most recently from f4100bb to cc30233 Compare January 12, 2016 16:58

waldenraines force-pushed the 12405 branch from cc30233 to 7b6c2b3 Compare January 12, 2016 19:42

jlsherrill reviewed Jan 13, 2016
View reviewed changes

jlsherrill mentioned this pull request Jan 13, 2016

Fixes #13205 - do not lock host on upload_package_profile and update … #5705

Merged

Fixes #12405: move packages from system to host.

7583295

http://projects.theforeman.org/issues/12405

waldenraines force-pushed the 12405 branch from 7b6c2b3 to 7583295 Compare January 15, 2016 14:14

waldenraines pushed a commit that referenced this pull request Jan 18, 2016

Merge pull request #5656 from waldenraines/12405

ccf6ed2

Fixes #12405: move packages from system to host.

waldenraines merged commit ccf6ed2 into Katello:master Jan 18, 2016

waldenraines deleted the 12405 branch January 18, 2016 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #12405: move packages from system to host. #5656

Fixes #12405: move packages from system to host. #5656

waldenraines commented Dec 8, 2015

ehelms Dec 14, 2015

waldenraines commented Dec 17, 2015

jlsherrill Dec 18, 2015

waldenraines commented Dec 19, 2015

jlsherrill commented Dec 20, 2015

waldenraines commented Jan 6, 2016

jlsherrill Jan 9, 2016

jlsherrill Jan 13, 2016

jlsherrill commented Jan 11, 2016

waldenraines commented Jan 12, 2016

waldenraines commented Jan 12, 2016

jlsherrill Jan 13, 2016

waldenraines Jan 15, 2016

jlsherrill commented Jan 13, 2016

waldenraines commented Jan 15, 2016

jlsherrill commented Jan 18, 2016

Fixes #12405: move packages from system to host. #5656

Fixes #12405: move packages from system to host. #5656

Conversation

waldenraines commented Dec 8, 2015

ehelms Dec 14, 2015

Choose a reason for hiding this comment

waldenraines commented Dec 17, 2015

jlsherrill Dec 18, 2015

Choose a reason for hiding this comment

waldenraines commented Dec 19, 2015

jlsherrill commented Dec 20, 2015

waldenraines commented Jan 6, 2016

jlsherrill Jan 9, 2016

Choose a reason for hiding this comment

jlsherrill Jan 13, 2016

Choose a reason for hiding this comment

jlsherrill commented Jan 11, 2016

waldenraines commented Jan 12, 2016

waldenraines commented Jan 12, 2016

jlsherrill Jan 13, 2016

Choose a reason for hiding this comment

waldenraines Jan 15, 2016

Choose a reason for hiding this comment

jlsherrill commented Jan 13, 2016

waldenraines commented Jan 15, 2016

jlsherrill commented Jan 18, 2016