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 EAP and Wildfly to allowed types for MW topology #2724

Merged
merged 2 commits into from
Dec 4, 2017

Conversation

karelhala
Copy link
Contributor

@karelhala karelhala commented Nov 14, 2017

Fixes missing EAP and Wildfly item in topology

When there are any providers with EAP or Wildfly server monitored, they were not displayed in topology view. This PR fixes such issue

UI changes

Before

screenshot-toplogypage-servers 2

After

screenshot from 2017-11-14 14-53-05

Waiting on core

BZ

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

@karelhala
Copy link
Contributor Author

@miq-bot add_label bug,gaprindashvili/yes, middleware
@abonas can you please assign this to someone to review it? Thank you.

@abonas
Copy link
Member

abonas commented Nov 14, 2017

@miq-bot add_label topology

@abonas
Copy link
Member

abonas commented Nov 14, 2017

the PR LGTM. one thing I'm wondering about though - is MiddlewareServer now still needed here after moving to more concrete eap/wildfly types (and displaying them specifically) that derive from MiddlewareServer?

@abonas
Copy link
Member

abonas commented Nov 14, 2017

should any unitests be adjusted to those specific types? afaik we have some jasmin tests with example json files and other sample inputs that should be updated

@karelhala
Copy link
Contributor Author

@abonas well the server which has hawkular running on is of MiddlewareServer type, so this one would not be visible in topology.

Sure unit tests should include these new classes as well, I'll add them right away.

@karelhala
Copy link
Contributor Author

Travis fail is relevant to ManageIQ/manageiq#16478

@abonas
Copy link
Member

abonas commented Nov 16, 2017

shouldn't this and this be also updated?

@abonas
Copy link
Member

abonas commented Nov 19, 2017

@karelhala have you seen my comment above regarding other potential tests that should be updated?

@karelhala
Copy link
Contributor Author

@abonas sorry, I missed your last comment. I'll check it out, since PR ManageIQ/manageiq#16478 is closed I can now focus on fixing tests.

@abonas
Copy link
Member

abonas commented Nov 21, 2017

@karelhala what's the status of this PR? thanks.

@abonas
Copy link
Member

abonas commented Nov 21, 2017

@miq-bot assign @martinpovolny

@karelhala
Copy link
Contributor Author

@abonas I am fixing the tests right now.

@karelhala karelhala force-pushed the eapWildflyTopology branch 3 times, most recently from 803f23d to ca5c6d1 Compare November 24, 2017 15:07
@@ -79,6 +95,14 @@
"source": "MiddlewareManager1",
"target": "MiddlewareServer1"
Copy link
Member

Choose a reason for hiding this comment

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

is this link still relevant in the current architecture with eap/wildfly? I mean will we get it on real env? Won't we get only wildfly/eap objects in the json, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I am not against removing these, however we still use MiddlewareServer for Hawkular server. So Whenever I spin hawkular standalone the server inside it will be of type MiddlewareServer. I tested this with hawkinit and it was still present.
However if you'd were to run hawkular provider on wildfly/eap solely by installing some packages and such (kinda hard to do) it will be of type wildfly/eap.

Copy link
Member

Choose a reason for hiding this comment

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

ok, as long as it appears then it should be present. the point here is to make sure this reflects a real env.

:kind => "MiddlewareServerEap",
:display_kind => "MiddlewareServerEap",
:miq_id => mw_server_eap.id,
:icon => match(/vendor-wildfly/),
Copy link
Member

Choose a reason for hiding this comment

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

isn't the icon vendor-eap here?

:target => "MiddlewareMessaging" + mw_messaging.compressed_id.to_s}
{
:source => "MiddlewareManager" + ems_hawkular.compressed_id.to_s,
:target => "MiddlewareServer" + mw_server.compressed_id.to_s
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not relevant atm, we'll get only eap/wildfly types

:target => "MiddlewareServerWildfly" + mw_server_wildfly.compressed_id.to_s
},
{
:source => "MiddlewareServer" + mw_server.compressed_id.to_s,
Copy link
Member

Choose a reason for hiding this comment

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

like above, I think all MiddlewareServer is not going to appear anymore, unless I got it wrong

@karelhala karelhala force-pushed the eapWildflyTopology branch 3 times, most recently from a5a25af to 561db64 Compare November 29, 2017 15:46
@karelhala karelhala force-pushed the eapWildflyTopology branch 3 times, most recently from ac6d0f5 to 654a938 Compare November 29, 2017 17:33
:kind => "MiddlewareServerEap",
:display_kind => "MiddlewareServerEap",
:miq_id => mw_server_eap.id,
:icon => match(/vendor-jboss-eap/),
Copy link
Member

Choose a reason for hiding this comment

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

is this the same as above?
https://github.com/ManageIQ/manageiq-ui-classic/pull/2724/files#diff-f32f59cd3f168fe8e6bb029d5b543f20R34
here it's vendor-jboss-eap and there it's just vendor-eap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct!

@abonas
Copy link
Member

abonas commented Nov 29, 2017

lgtm except for the question about regarding the icon diff in names

@abonas
Copy link
Member

abonas commented Nov 30, 2017

tests are failing but it looks like an env issue? can you rerun them?

@karelhala karelhala closed this Nov 30, 2017
@karelhala
Copy link
Contributor Author

Rerunning travis

@karelhala karelhala reopened this Nov 30, 2017
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

Checked commits karelhala/manageiq-ui-classic@25a0da6~...5962eae with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

spec/services/middleware_topology_service_spec.rb

@abonas
Copy link
Member

abonas commented Nov 30, 2017

@martinpovolny merge please?

@martinpovolny martinpovolny merged commit 7e2227a into ManageIQ:master Dec 4, 2017
@martinpovolny martinpovolny added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 4, 2017
simaishi pushed a commit that referenced this pull request Dec 4, 2017
Add EAP and Wildfly to allowed types for MW topology
(cherry picked from commit 7e2227a)
@simaishi
Copy link
Contributor

simaishi commented Dec 4, 2017

Gaprindashvili backport details:

$ git log -1
commit 49d901950fd7feb8b7e8de3cd70aa79b2af19493
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Mon Dec 4 15:08:00 2017 +0100

    Merge pull request #2724 from karelhala/eapWildflyTopology
    
    Add EAP and Wildfly to allowed types for MW topology
    (cherry picked from commit 7e2227a4ac3c6c9c25ad722035330cb8d734a700)

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