Skip to content

Conversation

@putmanoj
Copy link
Contributor

@putmanoj putmanoj commented Apr 1, 2025

This PR has changes to Service Reconfigure action, will following changes made.

  • adds /Service/Generic/StateMachines/GenericLifecycle/reconfigure entry point
  • modifies particularly, PreProcess method, to pass dialog params (changes from user), for reconfiguration process
  • adds /Service/Generic/StateMachines/Utils/util_object class, with common methods for service, service_action & update_task, to use in all the steps methods.
    • The main reason for adding the 'UtilObject' class is 'Reconfigure' action does not have 'service' & 'service_action' attributes, unlike in case of 'Provision' or 'Retirement' actions
    • task name attribute is different for each of the actions.
    • the methods are common methods used across the steps methods

Resolves https://jsw.ibm.com/browse/CP4AIOPS-5515

@Fryguy
Copy link
Member

Fryguy commented Apr 1, 2025

@putmanoj I assume this is mostly copied from another entrypoint, but which one (or is this all brand new)? I just wanted to see them side-by-side so I can understand what I'm looking at.

@Fryguy Fryguy self-assigned this Apr 1, 2025
@agrare agrare self-requested a review April 1, 2025 17:17
@putmanoj
Copy link
Contributor Author

putmanoj commented Apr 2, 2025

@putmanoj I assume this is mostly copied from another entrypoint, but which one (or is this all brand new)? I just wanted to see them side-by-side so I can understand what I'm looking at.

Hi @Fryguy
I will add all the PRs in the channel.
Also, updated the description, for better understanding.

Acutally, probably methods can be removed, we could use default methods, if ...

@putmanoj putmanoj changed the title [WIP] Add Generic Service Reconfigure entry point Add Generic Service Reconfigure entry point Apr 2, 2025
@putmanoj putmanoj force-pushed the support_service_reconfiguration_v4 branch from 81cafe4 to f25cf2e Compare April 4, 2025 11:55
Comment on lines 50 to 59
def reconfiguration_options
task = @handle.root["service_reconfigure_task"]
{:dialog => task.options[:dialog] || {}}
end

def update_options
if service_action == 'Reconfigure'
reconfiguration_options
else
{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: something about this doesn't seem right in the generic lifecycle preprocess, needs investigation (is there a reconfigure override we can stick this in?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agree, needs investigation, though for now, did not find anything better than this.

Comment on lines 18 to 36
def self.update_task(message, status = nil, handle = $evm)
task_name = if handle.root["request"] == "service_reconfigure"
'service_reconfigure_task'
else
'service_template_provision_task'
end
handle.root[task_name].try do |task|
task.miq_request.user_message = message
task.message = status unless status.nil?
end
end

def self.service(handle = $evm)
if handle.root["request"] == "service_reconfigure"
task = handle.root["service_reconfigure_task"]
service = task.source unless task.nil?
else
service = handle.root["service"]
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this kind of "is it provision or is it reconfigure" logic in a generic utils object also doesn't feel right

Copy link
Contributor Author

@putmanoj putmanoj Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created the utils object (probably could have named it 'common' object), was to hide this "is it provision or is it reconfigure" logic, from the step methods.

At present,

Also, would say there is bug in existing update_task method for Retirement, see https://github.com/putmanoj/manageiq-content/blob/master/content/automate/ManageIQ/Service/Generic/StateMachines/GenericLifecycle.class/__methods__/execute.rb#L34

              def update_task(message)
                @handle.root['service_template_provision_task'].try { |task| task.miq_request.user_message = message }
              end

though for Retirement the task_name is 'service_retire_task'

For Provision
Screenshot 2025-04-09 at 5 19 57 PM

For Reconfigure
Screenshot 2025-04-09 at 5 18 33 PM

For Retirement
Screenshot 2025-04-09 at 5 17 49 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with Automate, but I still feel like a lot of this if reconfigure else if retirement else could be replaced by attributes on the class that we could use instead

Comment on lines 34 to 35
if handle.root["request"] == "service_reconfigure"
task = handle.root["service_reconfigure_task"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set the service here could we drop this conditional? Seems weird that we have service for prov and retire but not reconfigure

when "service_retire"
"service_retire_task"
else
"service_template_provision_task"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is handle.root["request"] here? Is it service_template_provision? If so could we generically do #{handle.root["request"]}_task}?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll notice I'm trying to kill this whole util_object if we can get away with it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check images attached in #763 (comment)

  • Provision
    • request => "clone_to_service"
    • service_action => "Provision"
  • Reconfigure
    • request => "service_reconfigure"
    • service_action => (NOT AVAILABLE) - this is the main reason, need to add the util_object, unless we can fix this in core
  • Retirement
    • request => "service_retire"
    • service_action => "Retirement"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the task name, where patterns are different, between the actions

end
handle.root[task_name].try do |task|
task.miq_request.user_message = message
task.message = status unless status.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really minor but we tend to prefer the "positive" if status over the "negative" unless status.nil?

@miq-bot
Copy link
Member

miq-bot commented Apr 21, 2025

Checked commits putmanoj/manageiq-content@746e93a~...ca8c626 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint
20 files checked, 0 offenses detected
Everything looks fine. 👍

Comment on lines +43 to +48
def self.service(handle = $evm)
service = handle.root["service"] || service_task(handle)&.source
service.tap do |s|
ManageIQ::Automate::System::CommonMethods::Utils::LogObject.log_and_raise("Service not found", handle) if s.nil?
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@putmanoj with ManageIQ/manageiq#23405 merged do we still need this ?

@agrare agrare merged commit a2b65fc into ManageIQ:master Apr 21, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants