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

Remove middeware topology #3141

Merged
merged 1 commit into from
Jan 15, 2018
Merged

Remove middeware topology #3141

merged 1 commit into from
Jan 15, 2018

Conversation

abonas
Copy link
Member

@abonas abonas commented Dec 24, 2017 •
edited
Loading

Remove the implementation of middleware topology - ui and backend
@miq-bot add_label middleware

@miq-bot miq-bot added the wip label Dec 24, 2017
@abonas abonas force-pushed the removeMiddlewareTopology branch 2 times, most recently from fe36ebd to 552f190 Compare December 25, 2017 10:53
@abonas
Copy link
Member Author

abonas commented Dec 25, 2017

@martinpovolny I did not add you as a reviewer, not sure what automatic process did :)

@abonas abonas changed the title [WIP] Remove middeware topology Remove middeware topology Dec 25, 2017
@abonas
Copy link
Member Author

abonas commented Dec 25, 2017

@miq-bot rm_label wip

@abonas
Copy link
Member Author

abonas commented Dec 25, 2017

@miq-bot add_label gaprindashvili/no

@abonas
Copy link
Member Author

abonas commented Dec 25, 2017

@miq-bot add_label topology

@abonas
Copy link
Member Author

abonas commented Dec 25, 2017 •
edited
Loading

@karelhala @mtho11 please review and verify the topology is removed for middleware

@abonas abonas force-pushed the removeMiddlewareTopology branch 2 times, most recently from ad94601 to d406702 Compare December 25, 2017 13:04
@miq-bot
Copy link
Member

miq-bot commented Dec 25, 2017

Checked commit abonas@d406702 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 2 offenses detected

app/helpers/ems_middleware_helper/textual_summary.rb

app/views/layouts/_tabs.html.haml

  • ⚠️ - Line 56 - Convert if nested inside else to elsif.

@miq-bot
Copy link
Member

miq-bot commented Jan 5, 2018

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member

Please, rebase.

@israel-hdez israel-hdez mentioned this pull request Jan 9, 2018
Closed
@abonas
Copy link
Member Author

abonas commented Jan 10, 2018

rebased

@abonas
Copy link
Member Author

abonas commented Jan 10, 2018

tests fail around provider_foreman_controller_spec which is unrelated to this patch

@abonas
Copy link
Member Author

abonas commented Jan 10, 2018

@miq-bot rm_label unmergeable

@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2018

This pull request is not mergeable. Please rebase and repush.

@abonas
Copy link
Member Author

abonas commented Jan 11, 2018

tests failures seem unrelated

@abonas abonas closed this Jan 11, 2018
@abonas abonas reopened this Jan 11, 2018
@abonas
Copy link
Member Author

abonas commented Jan 11, 2018

rerunning tests

@abonas
Copy link
Member Author

abonas commented Jan 14, 2018

@martinpovolny ready for merge?

@abonas
Copy link
Member Author

abonas commented Jan 14, 2018

@karelhala please have a look on this as well. thanks

karelhala
karelhala reviewed Jan 15, 2018
View reviewed changes
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Looks good.

app/controllers/ems_middleware_controller.rb
%i(properties status),
%i(relationships smart_management)
]
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Member Author

@abonas abonas Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

excellent question :)
Without this part, it inherits textual_group_list from https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ems_common.rb#L24
which has topology defined.
And that causes opening several middleware screens to crash because "no method" etc.
So overriding it without the topology part solved the problem.

@martinpovolny martinpovolny merged commit ea401d5 into ManageIQ:master Jan 15, 2018
@martinpovolny martinpovolny added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 15, 2018
@israel-hdez israel-hdez mentioned this pull request Jan 23, 2018
Merged
@himdel himdel mentioned this pull request Jan 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karelhala karelhala karelhala left review comments

@martinpovolny martinpovolny Awaiting requested review from martinpovolny

Assignees
No one assigned
Labels
middleware topology
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abonas @miq-bot @martinpovolny @karelhala