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

Cap&U subclass MetricsCapture #19610

Merged
merged 6 commits into from
Dec 12, 2019
Merged

Cap&U subclass MetricsCapture #19610

merged 6 commits into from
Dec 12, 2019

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 7, 2019

This got bigger than I thought it would.

creates subclasses for MetricsCapture (currently around capture_ems_targets)
put those tests in appropriate place

Each type of ems captures different targets.
reduce specialized testing methods
this context is only used in one place
these are testing perf_capture_queue
group these using as priority tests
use existing miq_server / zone

use fewer @ variables
@kbrock kbrock changed the title [WIP] Cap&U tests Cap&U tests Dec 10, 2019
@kbrock kbrock changed the title Cap&U tests Cap&U subclass MetricsCapture Dec 10, 2019
@agrare agrare self-assigned this Dec 10, 2019
@@ -0,0 +1,5 @@
class ManageIQ::Providers::CloudManager::MetricsCapture < ManageIQ::Providers::BaseManager::MetricsCapture
def capture_ems_targets(options = {})
Metric::Targets.capture_cloud_targets([ems], options)
Copy link
Member

Choose a reason for hiding this comment

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

@kbrock is the goal to move all of e.g. capture_cloud_targets into the CloudManager::MetricsCapture class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, all MetricsCapture objects subclass BaseManager::MetricsCapture. So the base case will need to call into that method.

Are you thinking about moving that method into BaseManager::MetricsCapture and then removing/moving into the subclass once we are subclassing correctly?

I was thinking that we would introduce the subclassing. Change all providers. Then move/remove capture_cloud_targets.

@agrare
Copy link
Member

agrare commented Dec 10, 2019

This got bigger than I thought it would.

It really isn't bad, mostly moving specs around

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2019

Checked commits kbrock/manageiq@a344f84~...c7a663e with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
10 files checked, 3 offenses detected

spec/models/manageiq/providers/base_manager/metrics_capture_spec.rb

spec/models/manageiq/providers/cloud_manager/metrics_capture_spec.rb

Copy link
Member Author

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

I'd love to remove most from target.
Lets introduce the hiererchy and then drop them

@@ -0,0 +1,5 @@
class ManageIQ::Providers::CloudManager::MetricsCapture < ManageIQ::Providers::BaseManager::MetricsCapture
def capture_ems_targets(options = {})
Metric::Targets.capture_cloud_targets([ems], options)
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, all MetricsCapture objects subclass BaseManager::MetricsCapture. So the base case will need to call into that method.

Are you thinking about moving that method into BaseManager::MetricsCapture and then removing/moving into the subclass once we are subclassing correctly?

I was thinking that we would introduce the subclassing. Change all providers. Then move/remove capture_cloud_targets.

@agrare agrare merged commit 0cda5f6 into ManageIQ:master Dec 12, 2019
@agrare agrare added this to the Sprint 127 Ending Jan 6, 2020 milestone Dec 12, 2019
@kbrock kbrock deleted the cu_capture_tests branch December 13, 2019 04:58
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.

3 participants