Add object retirement_requester #18113

d-m-u · 2018-10-18T21:09:36Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638502

We should be setting the retirement_requester on the object, not just on the request.

related PRS:

Don't need the name, since it's mixed in... #18117 Please merge this as well. issues with CI today

don't backport me to g, please

but can be backported fine to h

d-m-u · 2018-10-18T21:11:29Z

@miq-bot add_label hammer/yes, bug, blocker, gaprindashvili/yes
but please don't backport, will need to open a whole new PR for gap
@miq-bot add_reviewer @tinaafitz
@miq-bot add_reviewer @bdunne
@miq-bot add_reviewer @kbrock

d-m-u · 2018-10-19T12:46:15Z

@miq-bot remove_label wip

tinaafitz

@d-m-u Looks good.
@mkanoor Please review.

d-m-u · 2018-10-19T13:55:49Z

@miq-bot assign @gmcculloug

gmcculloug · 2018-10-22T14:31:34Z

app/models/mixins/retirement_mixin.rb

+    def set_retirement_requester(obj_name, obj_ids, requester)
+      obj_ids.each do |id|
+        _log.info("Retirement requester for #{obj_name} #{id} being set to #{requester.userid}")
+        obj_name.constantize.find(id).update_attributes(:retirement_requester => requester.userid)


Have you verified that all object types that pass through this mixin logic have a retirement_requester? They need to be available on Hammer and Gaprindashvili branches as well.

I think that we're at least fine, haha. May 4th of '17 was the date that this field was available on Services n Vms n OrchStacks and LoadBalancers. Honestly, though, I have no idea why we even have retirement as a thing for load balancers, I don't think that's really a thing. But the column is there!

d-m-u · 2018-10-22T14:46:03Z

@miq-bot add_reviewer @mkanoor

d-m-u · 2018-10-22T15:14:31Z

Anyone? Anyone? I know you all are 👻 s but it's 12 lines of code.

kbrock

I like it - just possibly like a more efficient request to update the objects.

kbrock · 2018-10-22T15:03:33Z

spec/models/service/retirement_management_spec.rb

-    User.current_user = user
-    expect(ServiceRetireRequest).to receive(:make_request).with(nil, {:src_ids => ['yabadabadoo'], :__request_type__ => "service_retire"}, User.current_user, true)
-    @service.class.to_s.demodulize.constantize.make_retire_request('yabadabadoo', User.current_user)
+    expect(ServiceRetireRequest).to receive(:make_request).with(nil, {:src_ids => [service3.id], :__request_type__ => "service_retire"}, user, true)


ASIDE: (only if this is a quick fix)

these tests are making us not call make_request. Makes me a little nervous to hide this code.

But I guess this is what the existing code does.
Just curious if it is quick to do this another way.

kbrock · 2018-10-22T15:10:49Z

app/models/mixins/retirement_mixin.rb

@@ -26,6 +27,13 @@ def retire(ids, options = {})
      q_options.merge!(:user_id => user.id, :group_id => user.current_group.id, :tenant_id => user.current_tenant.id) if user
      MiqQueue.put(q_options)
    end
+
+    def set_retirement_requester(obj_name, obj_ids, requester)
+      obj_ids.each do |id|


This will be doing an N+1 for looking up object_ids. Not sure how many there will be. Or if some of the ids will be invalid.

obj_name.constatize.where(:id => obj_ids).each do |object| _log.info("Retirement requester for #{obj_name} #{object.id} being set to #{requester.userid}") object.update_attributes(:retirement_requester => requester.userid)) end

Not sure if you need to count the query result and raise a not found error or something

Also, can't remember the way we typically display class:id, but thought it had [] or something.

an update_all(:retirement_requester => requester.userid) would just do it in the database and not fetch any of the results - again, an optimization that may make sense or not.

Or

kbrock · 2018-10-22T15:14:29Z

spec/models/service/retirement_management_spec.rb

@@ -96,15 +99,13 @@
  end

  it "with one src_id" do
-    User.current_user = user
-    expect(ServiceRetireRequest).to receive(:make_request).with(nil, {:src_ids => ['yabadabadoo'], :__request_type__ => "service_retire"}, User.current_user, true)
-    @service.class.to_s.demodulize.constantize.make_retire_request('yabadabadoo', User.current_user)


thank you so much for removing this 'yabadabadoo' junk.
I like the Flinstones and all but... yea

kbrock · 2018-10-22T15:14:51Z

spec/models/service/retirement_management_spec.rb

  end

  it "with many src_ids" do
-    User.current_user = user
-    expect(ServiceRetireRequest).to receive(:make_request).with(nil, {:src_ids => [1, 2, 3], :__request_type__ => "service_retire"}, User.current_user, true)


again - thanks for removing these hardcoded ids - way too fragile

bdunne · 2018-10-22T17:21:20Z

app/models/mixins/retirement_mixin.rb

+    def set_retirement_requester(klass, obj_ids, requester)
+      existing_objects = klass.where(:id => obj_ids)
+      existing_objects.update_all(:retirement_requester => requester.userid)
+      if (obj_ids - existing_objects.pluck(:id)).present?


Maybe set the variable before the conditional?

kbrock

just minor tweaks to avoid downloading all the existing_objects

In this rendition, existing_objects are just a scope - not downloaded

kbrock · 2018-10-22T17:23:12Z

app/models/mixins/retirement_mixin.rb

+
+    def set_retirement_requester(klass, obj_ids, requester)
+      existing_objects = klass.where(:id => obj_ids)
+      existing_objects.update_all(:retirement_requester => requester.userid)


Suggested change

existing_objects.update_all(:retirement_requester => requester.userid)

update_count = existing_objects.update_all(:retirement_requester => requester.userid)

kbrock · 2018-10-22T17:23:37Z

app/models/mixins/retirement_mixin.rb

+    def set_retirement_requester(klass, obj_ids, requester)
+      existing_objects = klass.where(:id => obj_ids)
+      existing_objects.update_all(:retirement_requester => requester.userid)
+      if (obj_ids - existing_objects.pluck(:id)).present?


Suggested change

if (obj_ids - existing_objects.pluck(:id)).present?

if update_count != obj_ids.count

kbrock · 2018-10-22T17:24:44Z

app/models/mixins/retirement_mixin.rb

+        objects_not_found = (obj_ids - existing_objects.pluck(:id))
+        _log.info("Retirement requester for #{obj_name}.pluralize #{objects_not_found} not set because objects not found")
+      else
+        _log.info("Retirement requester for #{obj_name}.pluralize #{existing_objects} being set to #{requester.userid}")


Suggested change

_log.info("Retirement requester for #{obj_name}.pluralize #{existing_objects} being set to #{requester.userid}")

_log.info("Retirement requester for #{obj_name}.pluralize #{obj_ids} being set to #{requester.userid}")

miq-bot · 2018-10-22T17:50:52Z

Checked commit d-m-u@2168b96 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

app/models/mixins/retirement_mixin.rb

❗ - Line 33, Col 40 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

kbrock

LGTM

ASSUMPTION: there is no callback logic triggered by an object being retired.
(If there is callback logic, then we'll need to fallback to existing_objects.each { |o| ... })

d-m-u · 2018-10-22T18:41:13Z

@simaishi It's g/yes but can't be backported, will be opening another for the gap branch.

don't backport me to g, please

though h is fine...

Add object retirement_requester (cherry picked from commit aaf424f) https://bugzilla.redhat.com/show_bug.cgi?id=1638502

simaishi · 2018-10-22T20:58:11Z

Hammer backport details:

$ git log -1
commit c7715ec2ef78f24933420868fa2c1dc2fb2e4711
Author: Keenan Brock <[email protected]>
Date:   Mon Oct 22 14:39:22 2018 -0400

    Merge pull request #18113 from d-m-u/adding_object_retirement_requester
    
    Add object retirement_requester
    
    (cherry picked from commit aaf424f04def8ce40be7e639730782c9f8f38042)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1638502

simaishi · 2019-01-21T14:20:22Z

As per @d-m-u this PR is no longer needed in gaprindashvili branch, removing the label.

miq-bot added the wip label Oct 18, 2018

miq-bot added blocker bug gaprindashvili/yes labels Oct 18, 2018

miq-bot requested review from tinaafitz, bdunne and kbrock October 18, 2018 21:15

d-m-u force-pushed the adding_object_retirement_requester branch 3 times, most recently from 09341ab to ae70343 Compare October 19, 2018 12:44

d-m-u changed the title ~~[WIP] Add object retirement_requester~~ Add object retirement_requester Oct 19, 2018

miq-bot removed the wip label Oct 19, 2018

d-m-u mentioned this pull request Oct 19, 2018

retirement initiator #18110

Closed

d-m-u force-pushed the adding_object_retirement_requester branch from ae70343 to 14cb00b Compare October 19, 2018 13:36

tinaafitz approved these changes Oct 19, 2018

View reviewed changes

miq-bot assigned gmcculloug Oct 19, 2018

gmcculloug reviewed Oct 22, 2018

View reviewed changes

kbrock reviewed Oct 22, 2018

View reviewed changes

d-m-u force-pushed the adding_object_retirement_requester branch from 14cb00b to 94a72f8 Compare October 22, 2018 17:03

bdunne reviewed Oct 22, 2018

View reviewed changes

miq-bot requested a review from mkanoor October 22, 2018 17:24

kbrock reviewed Oct 22, 2018

View reviewed changes

Add object retirement_requester

2168b96

d-m-u force-pushed the adding_object_retirement_requester branch from 94a72f8 to 2168b96 Compare October 22, 2018 17:48

kbrock approved these changes Oct 22, 2018

View reviewed changes

mkanoor approved these changes Oct 22, 2018

View reviewed changes

kbrock assigned kbrock and unassigned gmcculloug Oct 22, 2018

kbrock added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 22, 2018

kbrock merged commit aaf424f into ManageIQ:master Oct 22, 2018

d-m-u mentioned this pull request Oct 22, 2018

Don't need the name, since it's mixed in... #18117

Merged

simaishi pushed a commit that referenced this pull request Oct 22, 2018

Merge pull request #18113 from d-m-u/adding_object_retirement_requester

c7715ec

Add object retirement_requester (cherry picked from commit aaf424f) https://bugzilla.redhat.com/show_bug.cgi?id=1638502

simaishi added hammer/backported and removed hammer/yes labels Oct 22, 2018

simaishi added the gaprindashvili/conflict label Nov 5, 2018

simaishi removed gaprindashvili/conflict labels Jan 21, 2019

d-m-u deleted the adding_object_retirement_requester branch February 1, 2019 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object retirement_requester #18113

Add object retirement_requester #18113

d-m-u commented Oct 18, 2018 •

edited by kbrock

Loading

d-m-u commented Oct 18, 2018

d-m-u commented Oct 19, 2018

tinaafitz left a comment

d-m-u commented Oct 19, 2018

gmcculloug Oct 22, 2018

d-m-u Oct 22, 2018 •

edited

Loading

d-m-u commented Oct 22, 2018

d-m-u commented Oct 22, 2018

kbrock left a comment

kbrock Oct 22, 2018

kbrock Oct 22, 2018

kbrock Oct 22, 2018

kbrock Oct 22, 2018

bdunne Oct 22, 2018

kbrock left a comment

kbrock Oct 22, 2018

kbrock Oct 22, 2018

kbrock Oct 22, 2018

miq-bot commented Oct 22, 2018

kbrock left a comment

d-m-u commented Oct 22, 2018 •

edited by kbrock

Loading

simaishi commented Oct 22, 2018

simaishi commented Jan 21, 2019

	existing_objects.update_all(:retirement_requester => requester.userid)
	update_count = existing_objects.update_all(:retirement_requester => requester.userid)

	if (obj_ids - existing_objects.pluck(:id)).present?
	if update_count != obj_ids.count

	_log.info("Retirement requester for #{obj_name}.pluralize #{existing_objects} being set to #{requester.userid}")
	_log.info("Retirement requester for #{obj_name}.pluralize #{obj_ids} being set to #{requester.userid}")

Add object retirement_requester #18113

Add object retirement_requester #18113

Conversation

d-m-u commented Oct 18, 2018 • edited by kbrock Loading

don't backport me to g, please

d-m-u commented Oct 18, 2018

d-m-u commented Oct 19, 2018

tinaafitz left a comment

Choose a reason for hiding this comment

d-m-u commented Oct 19, 2018

Choose a reason for hiding this comment

d-m-u Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

d-m-u commented Oct 22, 2018

d-m-u commented Oct 22, 2018

kbrock left a 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

kbrock left a 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

miq-bot commented Oct 22, 2018

kbrock left a comment

Choose a reason for hiding this comment

d-m-u commented Oct 22, 2018 • edited by kbrock Loading

don't backport me to g, please

simaishi commented Oct 22, 2018

simaishi commented Jan 21, 2019

d-m-u commented Oct 18, 2018 •

edited by kbrock

Loading

d-m-u Oct 22, 2018 •

edited

Loading

d-m-u commented Oct 22, 2018 •

edited by kbrock

Loading