Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update a service's lifecycle_state. #18803

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

lfu
Copy link
Member

@lfu lfu commented May 23, 2019

Add lifecycle_state column to services to record a service's provision state. May be extended to the full lifecycle which contains provision, reconfigure and retirement.

Depends on ManageIQ/manageiq-schema#374.
https://bugzilla.redhat.com/show_bug.cgi?id=1677571

@miq-bot assign @tinaafitz
@miq-bot add_label enhancement, changelog/yes, services

@bdunne
Copy link
Member

bdunne commented Jun 3, 2019

Are you going to deprecate_attribute in case there are other callers to the renamed column?

@lfu
Copy link
Member Author

lfu commented Jun 3, 2019

The renamed column was recently added in ManageIQ/manageiq-schema#356.
There isn't any usage yet.

end

def provisioned?
lifecycle_state == 'provisioned'
Copy link
Member

Choose a reason for hiding this comment

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

@lfu Would it make sense to have the states as constants and freeze them? (provisioning, provisioned, error_provisioned)

@@ -88,6 +89,7 @@ class Service < ApplicationRecord
default_value_for :display, false
default_value_for :retired, false
default_value_for :initiator, 'user'
default_value_for :lifecycle_state, 'unprovisioned'
Copy link
Member

Choose a reason for hiding this comment

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

@lfu I know we discussed using 'unprovisioned', but I'm wondering if there is another value we could use here?
@mkanoor Thoughts?

expect(@service.lifecycle_state).to eq('provisioned')
end

it 'set servcie lifecycle_state to error_provisioned' do
Copy link
Member

@tinaafitz tinaafitz Jun 5, 2019

Choose a reason for hiding this comment

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

@lfu same here.

def update_lifecycle_state
case miq_request.request_state
when "finished"
lifecycle_state = miq_request.status == 'Ok' ? "provisioned" : "error_provisioned"
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu should error_provisioned be error_in_provisioning? or can it be just error

Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor We had a meeting with @Fryguy where we discussed possibly using the lifecycle_state for retirement in the future, so we're trying to use 'provision' in the state where possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will take error_in_provisioning to support retirement in the future.

expect(@service.lifecycle_state).to eq('provisioning')
end

it 'set servcie lifecycle_state to provisioned' do
Copy link
Member

@tinaafitz tinaafitz Jun 5, 2019

Choose a reason for hiding this comment

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

@lfu Can you fix the typo for "set servcie"

@lfu lfu force-pushed the rename_service_state_1677571 branch from 7ea19a1 to f7cf34e Compare June 6, 2019 20:59
@Fryguy
Copy link
Member

Fryguy commented Jun 10, 2019

@lfu Dependent schema PR is merged.

@Fryguy Fryguy closed this Jun 10, 2019
@Fryguy Fryguy reopened this Jun 10, 2019
@lfu lfu force-pushed the rename_service_state_1677571 branch from f7cf34e to e53acfc Compare June 19, 2019 11:45
@task_0.destination = @service
@request.miq_request_tasks.except(@task_0).each { |t| t.update(:state => "finished") }
@task_0.update_and_notify_parent(:state => "finished", :status => "Ok", :message => "Test Message")
expect(@service.lifecycle_state).to eq(Service::STATE_PROVISIONED)
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu should this just use the helper method provisioned? ?

@task_0.destination = @service
@request.miq_request_tasks.except(@task_0).each { |t| t.update(:state => "finished") }
@task_0.update_and_notify_parent(:state => "finished", :status => "Error", :message => "Test Message")
expect(@service.lifecycle_state).to eq(Service::STATE_ERROR_PROVISIONING)
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu this could use the provision_failed? method

@lfu lfu force-pushed the rename_service_state_1677571 branch from e53acfc to 747f636 Compare June 20, 2019 14:28
@@ -258,6 +258,30 @@ def service_resource_id(index, scaling_max)
@task_1_2.destination = @service
@task_1_2.update_and_notify_parent(:state => "finished", :status => "Ok", :message => "Test Message")
end

it 'set service lifecycle_state to provisioning' do
zone = FactoryBot.create(:zone, :name => "service_template_zone")
Copy link
Member

Choose a reason for hiding this comment

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

Please don't set zone name, it is sequenced by the factory


it 'set service lifecycle_state to provisioning' do
zone = FactoryBot.create(:zone, :name => "service_template_zone")
@task_3.source = FactoryBot.create(:service_template, :zone => zone)
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to set zone here?

@lfu lfu force-pushed the rename_service_state_1677571 branch from 747f636 to da1c436 Compare June 20, 2019 14:54
@miq-bot
Copy link
Member

miq-bot commented Jun 20, 2019

Checked commit lfu@da1c436 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 👍

@bdunne bdunne merged commit fff2760 into ManageIQ:master Jun 21, 2019
@bdunne bdunne assigned bdunne and unassigned tinaafitz Jun 21, 2019
@bdunne bdunne added this to the Sprint 114 Ending Jun 24, 2019 milestone Jun 21, 2019
@lfu lfu deleted the rename_service_state_1677571 branch July 29, 2019 19:50
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.

6 participants