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

Rendering status for servers and deployments in middleware topology #1461

Merged

Conversation

israel-hdez
Copy link
Member

@israel-hdez israel-hdez commented May 30, 2017

Status of middleware servers and deployments are set when building
topology view. This renders the status in tooltips in related
topology nodes and also renders status colors.

Regarding colors:

  • For middleware servers, when status is:
    • Down => red
    • Running => green
    • Reload required => amber
    • Starting => blue
  • For deployments:
    • Enabled => green
    • Disabled => amber

This is the first step to cover ManageIQ/manageiq-providers-hawkular#5

topology_screenshot

@mtho11
Copy link
Contributor

mtho11 commented Jun 2, 2017

So 'Reload required' is the same color as 'Starting'. Shouldn't we distinguish between them or else users will be restarting servers that are just starting up?

@israel-hdez
Copy link
Member Author

@mtho11 Yes, both of them will show the same color. My thinking was using amber to show that something is different than a "normal" state. But you have a good point. May be I should make "starting" to render as gray (the same as unknown) or add a new color (blue is the one I'm thinking)?

@israel-hdez israel-hdez force-pushed the topology_servers_deployments_status branch from fe2a5e0 to fa401f2 Compare June 6, 2017 18:40
@israel-hdez
Copy link
Member Author

@mtho11 I changed "starting" to be blue.
@miq-bot assign @abonas

@abonas
Copy link
Member

abonas commented Jun 7, 2017

@miq-bot add_label enhancement, topology

@abonas
Copy link
Member

abonas commented Jun 7, 2017

@mtho11 I changed "starting" to be blue.
@miq-bot assign @abonas

is the screenshot reflecting that?

@abonas
Copy link
Member

abonas commented Jun 7, 2017

probably this test needs fixing too (and its data)

@israel-hdez israel-hdez force-pushed the topology_servers_deployments_status branch from fa401f2 to b948104 Compare June 7, 2017 22:27
@israel-hdez
Copy link
Member Author

@abonas

is the screenshot reflecting that?

It wasn't. Now it's updated. The only state I couldn't do is the "reload required" for servers that should be amber. For some reason, when the server is in "reload required" state, the agent reports it as "down" and becomes red in topology. I'll ask in IRC tomorrow in my morning to know if this is an issue or if that's how it works.

probably this test needs fixing too (and its data)

Ah! I missed that test. Thanks for pointing it. Hmm... But the data/fixture is only used to check presence of parsed data here and here. The other examples just test directly the javascript functions as a unit. No changes are required to existent tests, but topologyService is the one responsible to render colors, so I added tests to check that the right colors are being rendered for all different statuses of servers and deployments.

.kube-topology g circle.information {
stroke: #00c;
}

Copy link
Member

Choose a reason for hiding this comment

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

does it have to be in the general topology css?
we have a designated css for middleware topology here

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't need to be here. But the case statement selecting which css class will be rendered is in the general topology_service.js. I can move it to the middleware css and place a note in topology js stating where that style is defined and if somebody else needs it, she/he can move the style to the general topology css.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree with having the css in middleware topology as well.

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! Now it is in middleware css.

@@ -19,7 +19,8 @@
:feed => '70c798a0-6985-4f8a-a525-012d8d28e8a3',
:ems_ref => long_id_0,
:nativeid => 'Local~~',
:ext_management_system => ems_hawkular)
:ext_management_system => ems_hawkular,
:properties => { 'Calculated Server State' => 'running' })
Copy link
Member

Choose a reason for hiding this comment

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

is there a test checking what happens when the status property is not set?

Copy link
Member Author

Choose a reason for hiding this comment

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

There isn't. I will add it. It should render as "unknown" (gray circle).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, it is down in the file.

@abonas
Copy link
Member

abonas commented Jun 12, 2017

@miq-bot add_label middleware

@mtho11
Copy link
Contributor

mtho11 commented Jun 12, 2017

Please be aware of topology rewrite with angular components: ManageIQ/ui-components#75

Status of middleware servers and deployments are set when building
topology view. This renders the status in tooltips in related
topology nodes and also renders status colors.

Regarding colors:
- For middleware servers, when status is:
  * Down => red
  * Running => green
  * Reload required => amber
  * Starting => blue
- For deployments:
  * Enabled => green
  * Disabled => amber

This is the first step to cover ManageIQ/manageiq-providers-hawkular#5
@israel-hdez israel-hdez force-pushed the topology_servers_deployments_status branch from b948104 to 4108d2e Compare June 12, 2017 21:44
@miq-bot
Copy link
Member

miq-bot commented Jun 12, 2017

Checked commit israel-hdez@4108d2e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

"display_kind": "MiddlewareDeploymentWar"
}
};

var mw_deployment_disabled = {
id: "MiddlewareDeployment1",
Copy link
Member

Choose a reason for hiding this comment

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

it's a nit, but I would set different ids to different vars in the test. from a consistency perspective only.

@abonas
Copy link
Member

abonas commented Jun 13, 2017

other than a nit, lgtm and ready for merge.
@miq-bot assign @martinpovolny

@miq-bot miq-bot assigned martinpovolny and unassigned abonas Jun 13, 2017
@skateman
Copy link
Member

As @mtho11 mentioned, I'm currently refactoring the topology screens and moving it to a single angular component with some extra features. From my side it's okay to merge this as it's not so complex, but I'd like to know about such new features to keep up with the new component...

@abonas
Copy link
Member

abonas commented Jun 13, 2017

As @mtho11 mentioned, I'm currently refactoring the topology screens and moving it to a single angular component with some extra features. From my side it's okay to merge this as it's not so complex, but I'd like to know about such new features to keep up with the new component...

@skateman
the color indication existed for a long time in containers topology, so it's not a completely new feature for topology. But we can and will definitely make you aware of topology changes.

@skateman
Copy link
Member

@abonas thanks!

@martinpovolny martinpovolny added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 13, 2017
@martinpovolny martinpovolny merged commit c9e5ecb into ManageIQ:master Jun 13, 2017
@israel-hdez israel-hdez deleted the topology_servers_deployments_status branch June 21, 2017 17:10
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

6 participants