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

Add retired to service active states #18348

Merged

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 10, 2019

We validate the request state in

validates :request_state, :inclusion => { :in => %w(pending finished) + ACTIVE_STATES, :message => "should be pending, #{ACTIVE_STATES.join(", ")} or finished" }
but vms can be provisioned, so the list should include that, and services can be retired, so it should include that too.

Should've been a part of #18283.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1666834

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 10, 2019

@miq-bot add_reviewer @tinaafitz
@miq-bot add_reviewer @bdunne
@miq-bot add_reviewer @mkanoor
@miq_bot add_label bug

@d-m-u d-m-u force-pushed the adding_provisioned_to_vm_active_states branch from 6f1407b to a58047d Compare January 10, 2019 21:40
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2019

@miq-bot add_label bug
Don't have one yet, will update when I do.

@miq-bot miq-bot added the bug label Jan 11, 2019
@@ -1,6 +1,7 @@
class ServiceRetireRequest < MiqRetireRequest
TASK_DESCRIPTION = 'Service Retire'.freeze
SOURCE_CLASS_NAME = 'Service'.freeze
ACTIVE_STATES = %w(retired) + base_class::ACTIVE_STATES
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but "retired" doesn't sound like an active state, a better name for the validation to use might be VALID_STATES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I told her about this yesterday.

Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u @bdunne I know it looks odd, and we can rename it in the future, but retired is an active state.
Reconfigured and migrated are active states as well.

Most of our lifecycle state machines have multiple states for each of 3 phases:

  1. Pre-processing
  2. Execution/Check Execution <---- the state would be provisioned/retired/reconfigured/migrated here
  3. Post-processing <----- It still has to process these states before it can be finished.
    The state is "finished" at the end of the state machine.

@d-m-u d-m-u force-pushed the adding_provisioned_to_vm_active_states branch from a58047d to a6cc73c Compare January 11, 2019 17:49
@miq-bot
Copy link
Member

miq-bot commented Jan 11, 2019

Checked commit d-m-u@a6cc73c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@d-m-u d-m-u changed the title Add provisioned to vm active states Add retired to service active states Jan 11, 2019
@bdunne bdunne merged commit 35c0f17 into ManageIQ:master Jan 12, 2019
@bdunne bdunne self-assigned this Jan 12, 2019
@bdunne bdunne added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 12, 2019
@simaishi
Copy link
Contributor

@d-m-u assume hammer/yes?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 16, 2019

Yup, sorry @simaishi
@miq-bot add_label hammer/yes

@d-m-u d-m-u deleted the adding_provisioned_to_vm_active_states branch February 1, 2019 20:47
simaishi pushed a commit that referenced this pull request Feb 5, 2019
@simaishi
Copy link
Contributor

simaishi commented Feb 5, 2019

Hammer backport details:

$ git log -1
commit 5f1d45b83cafc7809a498a5f2098c760979f1333
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Sat Jan 12 08:34:48 2019 -0500

    Merge pull request #18348 from d-m-u/adding_provisioned_to_vm_active_states
    
    Add retired to service active states
    
    (cherry picked from commit 35c0f1787c52a38ae4dc8d220a93e58a84ad628b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1672700

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.

5 participants