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

Remove MiddlewareManager Alerts #16729

Merged
merged 5 commits into from Jan 16, 2018

Conversation

cfcosta
Copy link

@cfcosta cfcosta commented Jan 2, 2018

This removes all the MiddlewareServer/Hawkular alerts, while also removing the logic to process it. This is a part of the process of phasing out MiddlewareManager.

Relates to (just a small glimpse on what's being done on this front, there's other PRs not mentioned here):
#16698
ManageIQ/manageiq-ui-classic#3142
ManageIQ/manageiq-ui-classic#3120
#16701
#16712
ManageIQ/manageiq-ui-classic#3118

Screenshots

Before:

After:

@cfcosta
Copy link
Author

cfcosta commented Jan 2, 2018

Please note that I didn't test it manually because the Control Explorer is failing per #16730. Will do as soon as it is fixed.

@abonas @agrare can you review?

@abonas
Copy link
Member

abonas commented Jan 7, 2018

@cfcosta the blocking issue is resolved, so please verify the code before review.
@israel-hdez please review

@cfcosta
Copy link
Author

cfcosta commented Jan 9, 2018

@abonas Done, everything is working as expected. Alerts are not on the listing anymore.

@abonas
Copy link
Member

abonas commented Jan 9, 2018

@cfcosta please add a screenshot of the affected UI

Copy link
Member

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

It is quite fine. Is there any complementary PR? I understand this is removing MW alert conditions and also some backend code. But I don't understand how the MW nodes are being removed from the control explorer.

@cfcosta
Copy link
Author

cfcosta commented Jan 10, 2018

@israel-hdez you were right, I forgot to remove it from the listing on the constant. That is fixed now.

@abonas updated the description for screenshots. Is that ok or do you need others?

@abonas
Copy link
Member

abonas commented Jan 11, 2018

@cfcosta are the screenshots representing only the "after the change" state?

@abonas
Copy link
Member

abonas commented Jan 12, 2018

@cfcosta what's the status of this PR? is it ready for review/merge by core?

@abonas
Copy link
Member

abonas commented Jan 12, 2018

@miq-bot add_label gaprindashvili/yes

@lfu
Copy link
Member

lfu commented Jan 15, 2018

Should MiddlewareServer be removed from this line in this PR?

@cfcosta
Copy link
Author

cfcosta commented Jan 16, 2018

@abonas sorry for the delay. Yeah, those screenshots represent the final state. I'm updating it to include the Before as well. And it is ready to merge/review.

@lfu good catch, fixed.

@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2018

Checked commits cfcosta/manageiq@ac5d338~...bc205cc with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 1 offense detected

app/models/miq_alert.rb

@lfu
Copy link
Member

lfu commented Jan 16, 2018

@cfcosta Can you take a look at Travis failure?

@cfcosta
Copy link
Author

cfcosta commented Jan 16, 2018


  1) VmdbDatabaseConnection is blocked
     Failure/Error: raise "Lock is not blocking"
     
     RuntimeError:
       Lock is not blocking
     # ./spec/models/vmdb_database_connection_spec.rb:77:in `block (2 levels) in <top (required)>'

This is the failure, does not seem related, and I'm not sure what this is referring to.

@agrare
Copy link
Member

agrare commented Jan 16, 2018

@cfcosta going to bounce the test, we're discussing the error in the manageiq/core room on gitter

@agrare agrare closed this Jan 16, 2018
@agrare agrare reopened this Jan 16, 2018
@lfu
Copy link
Member

lfu commented Jan 16, 2018

@gmcculloug LGTM 👍

@agrare agrare merged commit e519292 into ManageIQ:master Jan 16, 2018
@agrare agrare added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 16, 2018
@cfcosta cfcosta deleted the remove-middleware-alerts branch January 17, 2018 01:45
@himdel
Copy link
Contributor

himdel commented Jan 17, 2018

This caused ui travis failures, fixed by ManageIQ/manageiq-ui-classic#3260 .

simaishi pushed a commit that referenced this pull request Jan 17, 2018
Remove MiddlewareManager Alerts
(cherry picked from commit e519292)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 6a9b471c0d25bf0d3b8bc69d5aae9bdb89b1e9ae
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Jan 16 13:11:37 2018 -0500

    Merge pull request #16729 from cfcosta/remove-middleware-alerts
    
    Remove MiddlewareManager Alerts
    (cherry picked from commit e5192926f735bea28b495afd6cba757e2c1d1a83)

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.

None yet

9 participants