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 topology view for a single physical infra provider #4158

Closed

Conversation

felipedf
Copy link
Member

@felipedf felipedf commented Jun 18, 2018

The goal here is to create a topology for a single physical infra provider

screenshot-localhost-3000-2018 06 18-17-04-23

@miq-bot miq-bot added the wip label Jun 18, 2018
@felipedf felipedf changed the title [WIP] Add a dashboard view for a physical infra provider [WIP] Add topology view for a single physical infra provider Jun 18, 2018
@felipedf felipedf force-pushed the single_physical_infra_topology branch from 298943a to 01542e1 Compare June 18, 2018 19:54
@felipedf felipedf changed the title [WIP] Add topology view for a single physical infra provider Add topology view for a single physical infra provider Jun 18, 2018
@miq-bot miq-bot removed the wip label Jun 18, 2018
Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

Some inconsistencies are being introduced, please fix them. Thanks 👍 🍻


:javascript
miq_bootstrap('.topology');
= render :partial => "ems_physical_infra/show_topology"
Copy link
Member

Choose a reason for hiding this comment

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

Why introducing this inconsistency? All the other topology views are using *_topology/show.html.haml file structure...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, what happened here was that ems_comon/_show.html.haml was rendering ems_container/show_topology which is specific to containers ( I guess no other provider show a topology for a single provider, at least ems_infra doesn't ). I changed that to make it more generic. Do you have any other suggestion to make it look better?


:javascript
miq_bootstrap('.topology');
= render :partial => "ems_container/show_topology"
Copy link
Member

Choose a reason for hiding this comment

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

What this PR has to do with containers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since ems_comon show be generic, and it was rendering a file specific to containers, I moved it to a partial so ems_comon/_show.html.haml could be more generic. I kept this file because the topology view (that shows every provider) still uses this file.

@felipedf felipedf force-pushed the single_physical_infra_topology branch from 01542e1 to 5a95a0a Compare June 19, 2018 19:57
@miq-bot
Copy link
Member

miq-bot commented Jun 19, 2018

Checked commit felipedf@5a95a0a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@skateman
Copy link
Member

I don't really like that you're referring only to the physical topology in the commit(s)/description and touching also the containers. In general containers are behaving a little inconsistent compared to the other topology screens and this should be eliminated in the future.

Single infra providers also have topology screens, just accessible from elsewhere:
screenshot from 2018-06-20 09-57-37

I am planning to unify the containers with the rest and put the topology icon next to the dashboard/summary as in containers, but only after we will have the new topology component from the new patternfly.

@felipedf
Copy link
Member Author

I see, you are right.
So I guess I should remove that topology icon for now. Agree?

@skateman
Copy link
Member

@felipedf yes, please do so. Try to follow the other topology screens in consistency and all should be okay 🍻

@felipedf
Copy link
Member Author

Closing this PR as it will be waiting on changes of the container topology view

@felipedf felipedf closed this Jun 26, 2018
@felipedf felipedf deleted the single_physical_infra_topology branch July 3, 2018 17:48
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

4 participants