Fixes #6184 - Dynflowizes system update #4865
Conversation
| @@ -143,8 +143,7 @@ def create | |||
| param :host_collection_ids, Array, :desc => N_("Specify the host collections as an array") | |||
| def update | |||
| system_params = system_params(params) | |||
| @system.update_attributes!(system_params) | |||
| @system.refresh_subscriptions if system_params[:autoheal] | |||
| sync_task(::Actions::Katello::System::Update, @system, system_params) | |||
| respond_for_update | |||
There was a problem hiding this comment.
should we check the status of the action? (make sure it didn't fail)
There was a problem hiding this comment.
Is there a need for checking the task status? When it fails it raises exception and when it's successful it passes.
There was a problem hiding this comment.
I thought if it failed in the run phase no exception would be raised. I'll verify this.
There was a problem hiding this comment.
Looks like you're correct, ignore me! 👍
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
|
Apologies for the delay, going to review this today. |
| @@ -220,8 +203,6 @@ def save_candlepin_orchestration | |||
| case orchestration_for | |||
| when :hypervisor | |||
| # it's already saved = do nothing | |||
| when :update | |||
| pre_queue.create(:name => "update candlepin consumer: #{self.name}", :priority => 3, :action => [self, :update_candlepin_consumer]) | |||
| end | |||
| end | |||
There was a problem hiding this comment.
Can't we delete this whole method 'save_candlepin_orchestration' and get rid of the before_save that calls it?
|
Seeing an issue when trying to update a system: I think app/lib/actions/candlepin/consumer/update.rb needs to inherit from Candlepin::Abstract like some of the other candlepin actions do? |
| end | ||
|
|
||
| def finalize | ||
| Resources::Candlepin::Consumer.refresh_entitlements(input[:uuid]) |
There was a problem hiding this comment.
I think this should be ::Katello::Resources::Candlepin::Consumer I got a 'uninitialized constant' error.
when changing that class reference for testing i also get: 'Katello::Errors::UserNotSet unauthenticated to call a backend engine' here.
There was a problem hiding this comment.
any reason you did this in finalize and not as part of run?
There was a problem hiding this comment.
Candlepin actions indeed need to inherit from Candlepin::Abstract, you were right about ::Katello::Resources::Candlepin::Consumer. Finalize seemed more fitting to me for the task, but run is apparently the way to go here
jlsherrill
added
Waiting on contributor
Needs testing
and removed
Needs re-review
Needs testing
Not yet reviewed
labels
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
| module Actions | ||
| module Pulp | ||
| module Consumer | ||
| class Update < Actions::EntryAction |
There was a problem hiding this comment.
I think the same issue here, this needs to inherit from Pulp::Abstract too. This seemed to work for me:
module Actions
module Pulp
module Consumer
class Update < Pulp::Abstract
input_format do
param :uuid, String
param :display_name, String
end
def plan(system)
plan_self(:uuid => system.uuid, :display_name => system.name)
end
def run
output[:response] = pulp_extensions.consumer.update(input[:uuid], :display_name => input[:name])
end
end
end
end
end
There was a problem hiding this comment.
everything else looks good. Feel free to squash all your commits together as you fix this and i think it'll be ready to merge.
jlsherrill
added
Waiting on contributor
Needs testing
Katello 2.1
and removed
Needs testing
Needs re-review
labels
adamruzicka
force-pushed
the
dynflow-system-update
branch
from
90e3d17
to
7d52202
Compare
adamruzicka
force-pushed
the
dynflow-system-update
branch
from
7d52202
to
729f9f4
Compare
|
[test] |
|
Merging, thanks @adamruzicka ! |
Fixes #6184 - Dynflowizes system update
No description provided.