metric rollups subcollection #33
Conversation
|
@miq-bot remove_label wip |
miq-bot
changed the title
|
Hi @jntullo thanks for this Enhancement. Just minor stuff,
Thank you. |
additional specs
| def self.query_metric_rollups(params) | ||
| validate_params(params) | ||
|
|
||
| params[:offset] ||= 0 |
There was a problem hiding this comment.
Not certain, but I don't see this or the following param being used anywhere
There was a problem hiding this comment.
@imtayadeway it's being used by paging
There was a problem hiding this comment.
Ah! That's quite surprising to me - that this service would mutate some input params for the caller - perhaps this could be handled closer to where paging happens, or at least outside of this service?
| end | ||
|
|
||
| def self.validate_params(params) | ||
| REQUIRED_PARAMS.each do |key| |
There was a problem hiding this comment.
Since this service is larger than one method, WDYT about making it a stateful object? If you did that, you could for instance extract each validation into its own private method, with intention-revealing names, etc..
| @@ -0,0 +1,19 @@ | |||
| describe Api::MetricRollupsService do | |||
| before do | |||
| allow(Settings.api).to receive(:metrics_default_limit).and_return(1000) | |||
There was a problem hiding this comment.
Is this stub important to the actual values returned, or more of a sane default? If the latter, I wonder if it might be better placed as an actual configuration in the test environment, in config/settings/test.yml?
| allow(Settings.api).to receive(:metrics_default_limit).and_return(1000) | ||
| end | ||
|
|
||
| it 'validates that all of the parameters are present' do |
There was a problem hiding this comment.
Since all these tests are about validation, I'm wondering it that is suggesting the service's single responsibility should be validation, and that the rest could be pulled out either into the caller, or another service?
There was a problem hiding this comment.
@imtayadeway its single responsibility is not about validation. I just felt that it was better to test that the validation is done here rather than add individual tests for the various subcollections. The various subcollections do test the other functionality though. I can alter the tests.
There was a problem hiding this comment.
I just felt that it was better to test that the validation is done here rather than add individual tests for the various subcollections.
Definitely! I guess I was just thinking that the tests are telling me that validation is important enough to be its own service. If there's a responsibility besides validation that was important enough to extract, I think that behaviour deserves to also be tested at this level. It's OK to have some redundancy between tests of different sizes, since I'd expect a request spec to cover integration, and unit tests to cover all possible paths through the service
|
Checked commits jntullo/manageiq-api@8f1eff4~...3433d26 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Thanks @jntullo for the updates. 👍 |
Adds Metric Rollups as a subcollection to services and vms
Usage
Required parameters
The following parameters are required to query metric rollups as a subcollection
Optional parameters:
Paging
limitto override)Examples
Minimum attributes:
GET /api/services/:id/metric_rollups?capture_interval=hourly&start_date=2017-08-01End date:
GET /api/services/:id/metric_rollups?capture_interval=daily&start_date=2017-08-01&end_date=2017-08-22Limit:
GET /api/services/:id/metric_rollups?capture_interval=daily&start_date=2017-08-01&end_date=2017-08-22&limit=2000Dates are in the format:
YYYY-MM-DDorYYYY-MM-DDTHH:MM:SSZ@miq-bot add_label wip, enhancement