Add provisioned? lifecycle status info to retireable? check

[d-m-u]

We need to factor in the lifecycle status when we ask a service if it's retireable. Contrary to previous suggestions, it makes no sense to use the supports_feature mixin, because that's a class check and this is specific to the instance. It may as well just be part of this check, as well.

I just wanted to start the conversation and get feedback and buy-in. I'm not taking into account places this is overridden yet cause I wanted opinions first.

start for https://bugzilla.redhat.com/show_bug.cgi?id=1595511

@miq-bot assign @tinaafitz

[miq-bot]

@d-m-u 'lfu, bdunne, gmcculloug' is an invalid reviewer, ignoring...

[d-m-u]

@miq-bot add_reviewer @lfu
@miq-bot add_reviewer @bdunne
@miq-bot add_reviewer @gmcculloug

[d-m-u]

@Fryguy, thoughts? Since you are likely to be vociferous about it...

[d-m-u]

Anyone other than Lucy want to look at this, please?

[gmcculloug]

Does the provisioned? check effect all services? Meaning, if it is not provisioned can we immediately say it cannot be retired?

def retireable?
  false unless provisioned?

  parent.present? ? true : type.present?
end

Otherwise, it seems odd that we jump to using type.present? if parent.present? is true but provisioned? is false.

I guess the other form of this method I might expect is:

  def retireable?
    if parent.present?
      provisioned?
    else
      type.present?
    end
  end

[d-m-u]

Does the provisioned? check effect all services?

It should! https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/lifecycle_mixin.rb#L18

[gmcculloug]

Right. So does that mean that you can immediately return false if the service is not provisioned? My first example.

[d-m-u]

Anything currently provisioned has a lifecycle state that isn't set, so we can't currently use this until a data migration (ManageIQ/manageiq-schema#392) to fix that gets in. I'm not sure if there are other cases than this that were missed, @lfu, thoughts?

[d-m-u]

If a service is in the middle of retirement and failed, is it still considered provisioned? (Tina said yes.) If someone went in and manually retired a vm from a service, is the service still considered provisioned? (Still yes.) Could you conceivably then have a service with all parts and pieces gone that is still "provisioned"? I think you can, and I understand that, but I don't particularly like it.

[gmcculloug]

I would suggest that we focus on the normal use-case (provision service, then retire service) first and look into what I would consider edge conditions as a follow up. Seems like it would overcomplicate this PR by trying to handle everything here.

But to add thoughts on the questions above, I would think we could write a method to validate if all the parts of a service are retired. Then this check would need to be run at the end of the retirement of a resource and the service could be processed at that time. Or it could be done as a schedule event, but without digging to deep into it I like the first approach better. I am sure there are more nuances here that would need to be flushed out. Again, this would be a follow up effort.

[tinaafitz]

Thanks for creating the data migration @d-m-u. I'm sure we'll encounter other edge conditions, but I can't think of anything else right now. @lfu Do you have any concerns?

The initial use case for the lifecycle state is to prevent retirement from being called/run while the service is being provisioned. I believe the code changes here satisfy that requirement.

[d-m-u]

@miq-bot remove_label wip
everyone else, please re-review?

[miq-bot]

Checked commit d-m-u@f377318 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

[bdunne]

@tinaafitz Can you take another look?

[tinaafitz]

@bdunne Looks good.

[bdunne]

Have the UI tests been run with this change? I suspect they will have tests around this

[d-m-u]

@bdunne gimme just a minute, please

[d-m-u]

Of the 11 failures I get, they're all

 8) Mixins::EmsCommon::Angular get_task_args openstack cloud returns connect options for openstack cloud AMQP tab
     Failure/Error: expect(@ems_cloud_controller.send(:get_task_args, 'ManageIQ::Providers::Openstack::CloudManager')).to eq(expected_connect_options)
     
       expected: ["v2:{k8Sm5ygvDAvvY5zkvev1ag==}", {"amqp_api_port"=>"5462", "amqp_hostname"=>"host_amqp", "amqp_security_protocol"=>"non_ssl", "amqp_userid"=>"xyz"}]
            got: ["v2:{6yyYgLW3nPt9T1wkYsOzJQ==}", {"amqp_api_port"=>"5462", "amqp_hostname"=>"host_amqp", "amqp_security_protocol"=>"non_ssl", "amqp_userid"=>"xyz"}]
     
       (compared using ==)
     
       Diff:
       @@ -1,4 +1,4 @@
       -["v2:{k8Sm5ygvDAvvY5zkvev1ag==}",
       +["v2:{6yyYgLW3nPt9T1wkYsOzJQ==}",

and not a one looks related to any of this code change.

[d-m-u]

I suspect they will have tests around this

I mean no they won't, the UI work for this hasn't been done yet.

[d-m-u]

@miq-bot add_label enhancement
I guess you know it's buggy when it has its own label, looking at you, Services.