Adds task to subservices for correct retirement completion #17912

d-m-u · 2018-08-27T19:36:05Z

This changes the signature of the create_retire_subtask method to include the task, per the conversation @tinaafitz and @mkanoor and I had a week or so ago.

It's a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1608958

Issue #17993 opened to track virtual attributes occasionally returned by attributes method.
#17996 opened by Keenan to fix 17993.
Tested with ~~17996~~, works as expected. Yay. So the workaround for excluding virtual attributes directly from the service task creation is in this PR ~~because I have no idea when 17996 will get in~~.

There's no point in opening the rails pr as mentioned because it's fixed in 5.2 and a bunch of the attribute stuff changed.

TODO: use changes in #17996 to not have to filter out virtual attrs from nh attr list.

... update: ~~17996~~ is merged and #18019 opened to clean up the now-defunct TODO.

d-m-u · 2018-09-11T18:16:30Z

@tinaafitz can you take a look at this please to make sure it's okay before I spend time fixing the tests?

d-m-u · 2018-09-13T20:20:41Z

@tinaafitz can you please take a look? I need to fix the blocker bug that depends on this change.

d-m-u · 2018-09-13T20:32:02Z

@miq-bot remove_label wip

tinaafitz · 2018-09-13T21:02:15Z

app/models/service_retire_task.rb

@@ -42,6 +41,7 @@ def create_retire_subtasks(parent_service)
      # Initial Options[:dialog] to an empty hash so we do not pass down dialog values to child services tasks
      nh['options'][:dialog] = {}
      new_task = create_task(svc_rsc, parent_service, nh)


@d-m-u We need to pass the task(as parent_task) into the create_task method.

Then, we should set the parent_task_id to the parent_task.id here:

manageiq/app/models/service_retire_task.rb

Line 58 in 352693b

:parent_task_id => id,

And, we should set the parent_task.miq_request_tasks accordingly:

manageiq/app/models/service_retire_task.rb

Line 60 in 352693b

new_task.source = svc_rsc.resource

parent_task.miq_request_tasks << new_task

tinaafitz

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

gmcculloug · 2018-09-14T20:17:20Z

@d-m-u Please review failing test.

gmcculloug · 2018-09-14T22:03:39Z

app/models/service_retire_task.rb

@@ -41,23 +40,25 @@ def create_retire_subtasks(parent_service)
      nh['options'] = options.except(:child_tasks)
      # Initial Options[:dialog] to an empty hash so we do not pass down dialog values to child services tasks
      nh['options'][:dialog] = {}
-      new_task = create_task(svc_rsc, parent_service, nh)
+      new_task = create_task(svc_rsc, parent_service, nh, parent_task)


@d-m-u The nh has is pulling in virtual columns which is causing the test failure:

Failure/Error: new_task = (svc_rsc.resource.type.demodulize + "RetireTask").constantize.new(nh) ActiveModel::MissingAttributeError: can't write unknown attribute `href_slug` # ./app/models/service_retire_task.rb:53:in `create_task' # ./app/models/service_retire_task.rb:43:in `block in create_retire_subtasks' ...

You can fix this by excluding the virtual columns on line 39 using self.class.virtual_attribute_names

nh = attributes.except("id", "created_on", "updated_on", "type", "state", "status", "message", *self.class.virtual_attribute_names)

d-m-u · 2018-09-17T13:24:53Z

@mkanoor can you please review?

d-m-u · 2018-09-18T12:56:00Z

The test failure addressed by the removal of the virtual attributes (see https://github.com/ManageIQ/manageiq/pull/17912/files#r217853506) is a TODO that needs to be addressed subsequently. It'd be nice to get this merged as is because it's a bug fix. I vote that we address the virtual attribute weirdness later, I'll open an issue for it. Input, @gmcculloug @tinaafitz @Fryguy ??

tinaafitz · 2018-09-18T14:52:31Z

@d-m-u I agree that we should address the virtual attribute issue separately.
@gmcculloug We really need these changes for Service Retirement as a Request to work properly.
Can we merge this PR?

d-m-u · 2018-09-18T16:08:51Z

The issue is #17993

d-m-u · 2018-09-18T16:12:30Z

@miq-bot add_label blocker

gmcculloug · 2018-09-18T19:16:28Z

Agreed. Since this is a blocker the suggested work-around to ensure we exclude virtual attributes from the new hash should be good for now.

@Fryguy any issue with this work-around while we try to address the deeper active_record bug?

NickLaMuro

Drive by comment

(@d-m-u pinged me earlier, so I got a bit nerd sniped...)

NickLaMuro · 2018-09-18T22:46:55Z

app/models/service_retire_task.rb

    parent_service.service_resources.collect do |svc_rsc|
      next unless svc_rsc.resource.try(:retireable?)
      # TODO: the next line deals with the filtering for provisioning
      # (https://github.com/ManageIQ/manageiq/blob/3921e87915b5a69937b9d4a70bb24ab71b97c165/app/models/service_template/filter.rb#L5)
      # which should be extended to retirement as part of later work
      # svc_rsc.resource_type != "ServiceTemplate" || self.class.include_service_template?(self, srr.id, parent_service)
-      nh = attributes.except("id", "created_on", "updated_on", "type", "state", "status", "message")
+      nh = attributes.except("id", "created_on", "updated_on", "type", "state", "status", "message", *self.class.virtual_attribute_names)


Just an alternative to @gmcculloug 's suggestion:

nh = attributes.slice(self.class.column_names) .except!("id", "created_on", "updated_on", "type", "state", "status", "message")

Just makes it so that the list you are starting with is always what is available as columns from the database, and then it excludes columns from there, in the rare case that someone has fetch this current instance of this object was fetched using a custom SELECT statement that included some other derived columns.

... Not that I would know anyone who would do that, for say, performance reasons or anything...

( ͡° ͜ʖ ͡°)

Note: I just used .except! here to not dup the hash a second time, but that is probably peanuts in the grand scheme of things.

d-m-u · 2018-09-20T19:57:32Z

@gmcculloug @NickLaMuro @anyone_with_more_power_than_me

gmcculloug · 2018-09-20T20:59:35Z

app/models/service_retire_task.rb

+
+      # TODO: use changes here: https://github.com/ManageIQ/manageiq/pull/17996 to not have to filter by col_names
+      # 17996 removes virtual attributes from @attrs list
+      nh = attributes.slice(self.class.column_names).except!("id", "created_on", "updated_on", "type", "state", "status", "message")


@d-m-u slice expects *keys not an array of names. (https://apidock.com/rails/Hash/slice)

You need nh = attributes.slice(*self.class.column_names)...

When I run the command the way it is here I get an empty hash. Now my concern is that tests are passing with invalid data. 😱

gmcculloug

Need to correct call to slice and import test validation based on https://github.com/ManageIQ/manageiq/pull/17912/files#r219315906

miq-bot · 2018-09-20T21:22:08Z

Checked commits d-m-u/manageiq@fd5f912~...75f18ee with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

This comment has been minimized.

Sign in to view

miq-bot added lifecycle/retirement wip labels Aug 27, 2018

This comment has been minimized.

Sign in to view

miq-bot requested a review from mkanoor August 27, 2018 19:37

gmcculloug added the bug label Aug 27, 2018

gmcculloug self-assigned this Aug 27, 2018

gmcculloug requested a review from tinaafitz August 27, 2018 19:38

d-m-u force-pushed the bz1608958 branch from 74f64a7 to 9b796a5 Compare August 27, 2018 19:42

d-m-u mentioned this pull request Aug 28, 2018

[WIP] Mark child services as retired #17784

Closed

d-m-u force-pushed the bz1608958 branch from b4b6ce4 to 352693b Compare September 13, 2018 20:31

d-m-u changed the title ~~[WIP] Remove doubled service bundle children retirement call from RAAR~~ Remove doubled service bundle children retirement call from RAAR Sep 13, 2018

miq-bot removed the wip label Sep 13, 2018

d-m-u force-pushed the bz1608958 branch from 352693b to b873679 Compare September 13, 2018 21:13

d-m-u mentioned this pull request Sep 13, 2018

[GAPRINDASHVILI] Remove doubled call to retire service parts in bundle #17881

Merged

tinaafitz suggested changes Sep 13, 2018

View reviewed changes

d-m-u force-pushed the bz1608958 branch 2 times, most recently from 2a74486 to 7d6e8c7 Compare September 14, 2018 13:48

tinaafitz approved these changes Sep 14, 2018

View reviewed changes

gmcculloug reviewed Sep 14, 2018

View reviewed changes

d-m-u force-pushed the bz1608958 branch from 7d6e8c7 to 3cb9175 Compare September 17, 2018 11:34

mkanoor approved these changes Sep 17, 2018

View reviewed changes

d-m-u changed the title ~~Remove doubled service bundle children retirement call from RAAR~~ [WIP] Remove doubled service bundle children retirement call from RAAR Sep 17, 2018

miq-bot added the wip label Sep 17, 2018

d-m-u mentioned this pull request Sep 18, 2018

@attributes between Rails 5.0.0 and 5.0.1 includes virtual attributes #17993

Closed

miq-bot added the blocker label Sep 18, 2018

NickLaMuro reviewed Sep 18, 2018

View reviewed changes

d-m-u force-pushed the bz1608958 branch from 94769c3 to 763f54b Compare September 19, 2018 03:25

Remove doubled service bundle children retirement call from RAAR

fd5f912

d-m-u force-pushed the bz1608958 branch from 763f54b to 2047743 Compare September 19, 2018 13:13

kbrock mentioned this pull request Sep 19, 2018

update attribute_builder for rails change #17996

Merged

d-m-u changed the title ~~Remove doubled service bundle children retirement call from RAAR~~ Adds task to subservices for correct retirement completion Sep 19, 2018

d-m-u force-pushed the bz1608958 branch 6 times, most recently from b483770 to 88f5cf0 Compare September 20, 2018 19:15

gmcculloug reviewed Sep 20, 2018

View reviewed changes

gmcculloug suggested changes Sep 20, 2018

View reviewed changes

Test removal of doubled service child retirement call

75f18ee

d-m-u force-pushed the bz1608958 branch from 88f5cf0 to 75f18ee Compare September 20, 2018 21:19

gmcculloug approved these changes Sep 21, 2018

View reviewed changes

gmcculloug merged commit 8d4bf78 into ManageIQ:master Sep 21, 2018

gmcculloug added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 21, 2018

d-m-u mentioned this pull request Sep 25, 2018

Remove todo with virtual attr inclusion in attr list #18019

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds task to subservices for correct retirement completion #17912

Adds task to subservices for correct retirement completion #17912

d-m-u commented Aug 27, 2018 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

d-m-u commented Sep 11, 2018

d-m-u commented Sep 13, 2018

d-m-u commented Sep 13, 2018

tinaafitz Sep 13, 2018

tinaafitz left a comment

gmcculloug commented Sep 14, 2018

gmcculloug Sep 14, 2018

d-m-u commented Sep 17, 2018

d-m-u commented Sep 18, 2018 •

edited

Loading

tinaafitz commented Sep 18, 2018

d-m-u commented Sep 18, 2018

d-m-u commented Sep 18, 2018

gmcculloug commented Sep 18, 2018

NickLaMuro left a comment

NickLaMuro Sep 18, 2018

d-m-u commented Sep 20, 2018 •

edited

Loading

gmcculloug Sep 20, 2018

gmcculloug left a comment

miq-bot commented Sep 20, 2018

Adds task to subservices for correct retirement completion #17912

Adds task to subservices for correct retirement completion #17912

Conversation

d-m-u commented Aug 27, 2018 • edited Loading

TODO: use changes in #17996 to not have to filter out virtual attrs from nh attr list.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

d-m-u commented Sep 11, 2018

d-m-u commented Sep 13, 2018

d-m-u commented Sep 13, 2018

tinaafitz Sep 13, 2018

Choose a reason for hiding this comment

tinaafitz left a comment

Choose a reason for hiding this comment

gmcculloug commented Sep 14, 2018

gmcculloug Sep 14, 2018

Choose a reason for hiding this comment

d-m-u commented Sep 17, 2018

d-m-u commented Sep 18, 2018 • edited Loading

tinaafitz commented Sep 18, 2018

d-m-u commented Sep 18, 2018

d-m-u commented Sep 18, 2018

gmcculloug commented Sep 18, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Sep 18, 2018

Choose a reason for hiding this comment

d-m-u commented Sep 20, 2018 • edited Loading

gmcculloug Sep 20, 2018

Choose a reason for hiding this comment

gmcculloug left a comment

Choose a reason for hiding this comment

miq-bot commented Sep 20, 2018

d-m-u commented Aug 27, 2018 •

edited

Loading

d-m-u commented Sep 18, 2018 •

edited

Loading

d-m-u commented Sep 20, 2018 •

edited

Loading