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

Make disconnect_storage a no-op #272

Merged
merged 1 commit into from
Nov 28, 2018
Merged

Make disconnect_storage a no-op #272

merged 1 commit into from
Nov 28, 2018

Conversation

agrare
Copy link
Member

@agrare agrare commented Nov 15, 2018 •
edited
Loading

Disconnect_storage no longer needs to be called from the event_handler
and can be removed. Making these no-ops for backport.

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

@agrare agrare mentioned this pull request Nov 15, 2018
Merged
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1733

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.009%) to 80.571%
Files with Coverage Reduction New Missed Lines %
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb 1 91.22%
Totals Coverage Status
Change from base Build 1705: 0.009%
Covered Lines: 5225
Relevant Lines: 6485
💛 - Coveralls

@coveralls
Copy link

coveralls commented Nov 15, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 1767

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 80.586%
Totals Coverage Status
Change from base Build 1705: 0.02%
Covered Lines: 5226
Relevant Lines: 6485
💛 - Coveralls

@gmcculloug gmcculloug self-assigned this Nov 16, 2018
gmcculloug
gmcculloug reviewed Nov 16, 2018
View reviewed changes
lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_builtin_method.rb Outdated
def self.miq_src_vm_disconnect_storage(obj, _inputs)
event_object_from_workspace(obj).src_vm_disconnect_storage
def self.miq_src_vm_disconnect_storage(_obj, _inputs)
# Method has been deprecated and will be removed in a future version
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we are using the word "deprecated" correctly here. Would be more accurate to say that the src_vm_disconnect_storage logic has been removed and this method remains for backwards compatibility with older Automate models and/or users overriding DestroyVM_Task_Complete and USER_REMOVE_VM_FINISHED event instances. See ManageIQ/manageiq-content#472

The "will be removed in a future version" is right on. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, updated

@agrare agrare force-pushed the disconnect_storage_no_op branch from 6c10eba to 05c125d Compare November 17, 2018 00:40
gmcculloug
gmcculloug reviewed Nov 26, 2018
View reviewed changes
lib/miq_automation_engine/service_models/miq_ae_service_ems_event.rb Outdated
@@ -33,7 +33,7 @@ def src_vm_destroy_all_snapshots
end

def src_vm_disconnect_storage
ar_method { @object.src_vm_disconnect_storage }
# Method has been deprecated and will be removed in a future version
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs the same change.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done

@agrare
Make disconnect_storage a no-op
b0236ac 
Disconnect_storage no longer needs to be called from the event_handler
and can be removed.  Making these no-ops for backport.
@agrare agrare force-pushed the disconnect_storage_no_op branch from 05c125d to b0236ac Compare November 26, 2018 13:37
@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2018

Some comments on commit agrare@b0236ac

spec/service_models/miq_ae_service_ems_event_spec.rb

  • ⚠️ - 180 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2018

Checked commit agrare@b0236ac with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare
Copy link
Member Author

agrare commented Nov 28, 2018

Okay @gmcculloug this is ready

@agrare agrare mentioned this pull request Nov 28, 2018
Merged
@JPrause
Copy link
Member

JPrause commented Nov 28, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Nov 28, 2018

@agrare can you add the hammer/yes and gaprindashvili/yes labels if this can and should be backported.

@gmcculloug gmcculloug merged commit d9dcef8 into ManageIQ:master Nov 28, 2018
@agrare agrare deleted the disconnect_storage_no_op branch November 28, 2018 19:05
simaishi pushed a commit that referenced this pull request Nov 28, 2018
@gmcculloug @simaishi
Merge pull request #272 from agrare/disconnect_storage_no_op
8d0a459 
Make disconnect_storage a no-op

(cherry picked from commit d9dcef8)

https://bugzilla.redhat.com/show_bug.cgi?id=1654436
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 8d0a4592908eb8b24891ef567e460388942b5204
Author: Greg McCullough <[email protected]>
Date:   Wed Nov 28 14:04:56 2018 -0500

    Merge pull request #272 from agrare/disconnect_storage_no_op
    
    Make disconnect_storage a no-op
    
    (cherry picked from commit d9dcef8ce15f9b51766bed4ef65c41397a900122)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1654436

simaishi pushed a commit that referenced this pull request Nov 30, 2018
@gmcculloug @simaishi
Merge pull request #272 from agrare/disconnect_storage_no_op
449522d 
Make disconnect_storage a no-op

(cherry picked from commit d9dcef8)

https://bugzilla.redhat.com/show_bug.cgi?id=1644770
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 449522d7a70cf5cc7eda82598ab8279011c4c459
Author: Greg McCullough <[email protected]>
Date:   Wed Nov 28 14:04:56 2018 -0500

    Merge pull request #272 from agrare/disconnect_storage_no_op
    
    Make disconnect_storage a no-op
    
    (cherry picked from commit d9dcef8ce15f9b51766bed4ef65c41397a900122)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1644770

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

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@gmcculloug gmcculloug

Labels
blocker bug gaprindashvili/backported hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@agrare @coveralls @miq-bot @JPrause @simaishi @gmcculloug