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.

Sign up for GitHub

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

Jump to bottom

Automate - Added finish retirement notification. #14780

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Automate - Added finish retirement notification. #14780

merged 2 commits into from
Apr 18, 2017

Conversation

billfitzgerald0120
Copy link
Contributor

Updated all 5 methods with finish retirement tests.

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

@miq-bot add_label bug, fine/yes, automate

@billfitzgerald0120
Copy link
Contributor Author

@tinaafitz
Please review

@tinaafitz
Copy link
Member

@billfitzgerald0120 Looks good.
@mkanoor Please review.

@mkanoor
Copy link
Contributor

mkanoor commented Apr 17, 2017 •
edited
Loading

@tinaafitz @billfitzgerald0120
Do we need a notification when we start retirement?

I see that we create a notification at the start of a state machine
https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/Service/Generic/StateMachines/GenericLifecycle.class/__methods__/start.rb#L18

So the start is happening in the automate method side but the finish is in the app/models side.

bdunne
bdunne requested changes Apr 18, 2017
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

Requesting changes since it seems like a notification will always be created. Let me know if that is not the case.

spec/lib/miq_automation_engine/service_models/miq_ae_service_service_spec.rb Outdated
@@ -154,6 +154,7 @@ def invoke_ae
end

it "#finish_retirement" do
allow(Notification).to receive(:create)
Copy link
Member

Choose a reason for hiding this comment

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

Is a notification only created sometimes? If it is always created, these should use expect.

@tinaafitz
Copy link
Member

@mkanoor We should add notifications to model methods when available, but there is no model method for start.

@miq-bot
Copy link
Member

miq-bot commented Apr 18, 2017

Checked commits billfitzgerald0120/manageiq@0dd48a1~...6d66a1a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks good. 🍰

@billfitzgerald0120
Copy link
Contributor Author

@bdunne A notification will always be created so I changed the allow to expect in all 5 tests.
Please Review

bdunne
bdunne approved these changes Apr 18, 2017
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 Thanks @billfitzgerald0120

@bdunne bdunne merged commit 5164cd8 into ManageIQ:master Apr 18, 2017
@bdunne bdunne added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 18, 2017
@billfitzgerald0120 billfitzgerald0120 deleted the retirement_mixin branch April 19, 2017 14:25
simaishi pushed a commit that referenced this pull request Apr 20, 2017
@bdunne @simaishi
Merge pull request #14780 from billfitzgerald0120/retirement_mixin
64bdb46 
Automate - Added finish retirement notification.
(cherry picked from commit 5164cd8)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 64bdb46c9bc1ca4f53c8c56b2e6727786d8b2789
Author: Brandon Dunne <[email protected]>
Date:   Tue Apr 18 15:42:11 2017 -0400

    Merge pull request #14780 from billfitzgerald0120/retirement_mixin
    
    Automate - Added finish retirement notification.
    (cherry picked from commit 5164cd87dd1c55051704dc93a89fcd4dfbcdab21)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne approved these changes

Assignees

@gmcculloug gmcculloug

Labels
automate bug fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@billfitzgerald0120 @tinaafitz @mkanoor @miq-bot @simaishi @bdunne @gmcculloug