Create a new version of consumption, with preview budget merge with usage detail and reservation #2289

bgsky · 2018-01-18T20:22:11Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…tion

marstr · 2018-01-19T17:03:45Z

We made some updates in December, and moved back to working out of master. current is just a duplicate of master to allow for an easier transition. You can read more about all of the changes we made here: December 2017 Refactoring

bgsky · 2018-01-19T18:31:50Z

I saw you switched base branch to master. Other than that, let me know what to do with branch on my side and if you have any comments on this pull request.

marstr · 2018-01-19T19:05:31Z

I was just swooping in to make sure the PR was targeting the correct branch. I'll hand this back-off to the assignee, @alvadb.

This reverts commit 9010e29.

sandeepnl · 2018-01-23T00:17:21Z

@alvadb @salameer this PR is been pending for more than a day here. Can someone please review this soon?

jhendrixMSFT

The model validation failures need to be fixed, please see the travis log.

jhendrixMSFT · 2018-01-23T00:28:54Z

@bgsky has this been reviewed and signed off by ARM?

AutorestCI · 2018-01-23T00:28:56Z

Was unable to find SDK Azure/azure-sdk-for-python PR for this closed PR.

bgsky · 2018-01-23T00:36:12Z

@jhendrixMSFT This is merge of previous preview version with stable version. We don't have a ARM review for this pull request yet. Let me fix the errors first.

deprecatedAccount1 · 2018-01-23T03:26:38Z

Re-assigning to @jhendrixMSFT

jhendrixMSFT · 2018-01-23T17:18:23Z

Can you please provide a link to the preview version from which this swagger originates? Can you also please provide a reference to the PR for the preview version where ARM team signed off?

bgsky · 2018-01-23T18:26:22Z

Please refer to the following link for the preview version:

#2144

sandeepnl · 2018-01-23T18:35:51Z

@jhendrixMSFT @salameer

To be clear on the ARM review, this PR is publishing a new version of the api with no changes than what is already GA’ed in our ‘2017-11-30’ api version. We had got it reviewed with ARM team then and nothing has changed on it since then. Should we still get another approval from ARM for this too?

Essentially we are combining the operations available in our swagger versions ‘2017-11-30’ and ‘2017-12-30-preview’ into this.

Please let us know

jhendrixMSFT · 2018-01-23T18:43:46Z

@sandeepnl If the ARM team has signed off on the changes for preview then as long as this content is 100% identical there is no need for another review with ARM. It is good to have all the links to previous PRs with ARM's signoff however in #2144 it would appear that the ARM sign-off happened offline as I see no record of it in the PR.

jhendrixMSFT · 2018-01-23T18:50:31Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-01-31/consumption.json

@@ -853,7 +853,7 @@
      "type": "object",
      "allOf": [
        {
-          "$ref": "#/definitions/ProxyResource"
+          "$ref": "#/definitions/Resource"


Was this change intentional?

jhendrixMSFT · 2018-01-23T18:52:45Z

I merged the two swaggers by hand and diff'ed them with what's presented here and there are a few discrepancies.

Path "/providers/Microsoft.Consumption/operations" is missing in this new version
Type ProxyResource was renamed to Resource
The definition for Resource has changed, specifically the "eTag" field was replaced with the "tags" field, why is that?

bgsky · 2018-01-23T18:58:40Z

Thanks for merging and comparing. Will fix errors and send another iteration

sandeepnl · 2018-01-23T19:05:12Z

@bgsky We should have the operations path in the new swagger as well. Please add them and let us talk internally about why etags was removed.
@jhendrixMSFT Thanks for comparing and letting us know.

bgsky · 2018-01-23T22:33:52Z

@jhendrixMSFT Please lets know if model validation works.

This reverts commit 15221bd.

sandeepnl · 2018-01-23T23:02:29Z

...on/resource-manager/Microsoft.Consumption/stable/2017-11-30/examples/UsageDetailsExpand.json

@@ -4,7 +4,7 @@
    "scope": "subscriptions/subid/providers/Microsoft.Billing/billingPeriods/201702",
    "$expand": "meterDetails,additionalProperties",
    "$filter": "usageEnd le 2017-02-14T00:00:00Z",
-    "$top": "1"
+    "$top": 1


Dont change this version of the swagger/examples. You need to change it in 2018-01-31 version

Thanks for catch. This change is reverted, and new one on latest version is added

jhendrixMSFT · 2018-01-23T23:55:20Z

@dsgouda do you know why this model validation is failing? It says "expected type integer but found type string" however the type in the example is type integer. Is this a bug or am I missing something?

dsgouda · 2018-01-24T00:05:35Z

@jhendrixMSFT are these introduced in the current PR? I can't see anything in consumption.json.
I'd say @amarzavery or @mcardosos might have a better idea about model validator.

jhendrixMSFT · 2018-01-24T00:08:36Z

Yep this is current, here's the travis log. The operation ID in question is UsageDetails_List, example UsageDetailsExpand.

dsgouda · 2018-01-24T00:18:01Z

😆 I see what's wrong, OpenAPI specs do not recognize integer as a type, use number instead
Reference here

bgsky · 2018-01-24T00:20:39Z

Would it require the type change on our side? Lets change it to number on our side

bgsky · 2018-01-24T00:50:30Z

Travis CI build does not start automatically after the latest iteration. Is there an issue somewhere?

dsgouda · 2018-01-24T00:54:16Z

Taking a look.

azuresdkciprbot · 2018-01-24T01:01:53Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/consumption/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

jhendrixMSFT · 2018-01-24T16:30:06Z

@dsgouda That link is for OpenAPI 3.0 which AutoRest doesn't support (yet). Here's the link to the 2.0 spec. Integer is a valid type and is used in numerous swaggers. I think the bug is elsewhere, likely the model validator. Who owns that?

jhendrixMSFT · 2018-01-24T16:32:02Z

I would also add that changing it to number still fails model validation in the same way but with the number type, "Expected type number but found type string".

dsgouda · 2018-01-24T18:02:34Z

You're right @jhendrixMSFT my bad.
Yes, looks like a model validator bug after all. @mcardosos and @veronicagg own the model validator AFAIK.

jhendrixMSFT · 2018-01-24T18:08:41Z

@bgsky can you please revert your latest commit so they type is changed back to number? Sorry for the confusion.
@mcardosos is this a known issue with the model validator?

bgsky · 2018-01-24T18:29:29Z

@jhendrixMSFT My latest commit is to switch integer to number. It was originally integer, and is number now. Would you want to revert it?

jhendrixMSFT · 2018-01-24T18:42:41Z

Yes the type should be integer (number is used for floating point/decimal values).

This reverts commit 380cc0e.

azuresdkciprbot · 2018-01-24T18:57:48Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/consumption/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

jhendrixMSFT · 2018-01-24T19:08:29Z

I think the model validation failure is a bug, I've opened Azure/oav#197 to track it.

AutorestCI · 2018-01-24T19:10:08Z

Was unable to find SDK Azure/azure-sdk-for-python PR for this closed PR.

AutorestCI · 2018-01-24T19:19:45Z

No modification for AutorestCI/azure-sdk-for-python

lmazuel · 2018-01-25T17:46:25Z

@jhendrixMSFT This contains a global "budgetName" that should be a method declaration.
Azure/azure-openapi-validator#84

jhendrixMSFT · 2018-01-25T17:59:00Z

@lmazuel All I see is budgetNameParameter which is in the path. Am I missing something?

REDMOND\chanwan added 2 commits January 17, 2018 16:18

Create a new version, with budget merge with usage detail and reserva…

b8ae97d

…tion

Remove budget preview tag

044b7ba

azuresdkci assigned alvadb Jan 18, 2018

update readme with new version

66b4fb0

marstr changed the base branch from current to master January 19, 2018 17:02

REDMOND\chanwan added 2 commits January 19, 2018 14:15

Add resource group, resource, and filter to budget

9010e29

Revert "Add resource group, resource, and filter to budget"

4d061f3

This reverts commit 9010e29.

jhendrixMSFT requested changes Jan 23, 2018

View reviewed changes

jhendrixMSFT closed this Jan 23, 2018

jhendrixMSFT reopened this Jan 23, 2018

salameer assigned jhendrixMSFT and unassigned alvadb Jan 23, 2018

jhendrixMSFT added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 23, 2018

jhendrixMSFT removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 23, 2018

jhendrixMSFT reviewed Jan 23, 2018

View reviewed changes

Revert "Update data type to validate model"

abde435

This reverts commit 15221bd.

sandeepnl reviewed Jan 23, 2018

View reviewed changes

Update model to fix errors on model validation

ee76c77

Switch a data type from integer to number

380cc0e

Revert "Switch a data type from integer to number"

7fd01c1

This reverts commit 380cc0e.

jhendrixMSFT mentioned this pull request Jan 24, 2018

Model validation incorrectly fails on request parameter Azure/oav#197

Closed

jhendrixMSFT approved these changes Jan 24, 2018

View reviewed changes

jhendrixMSFT merged commit 90eb6bc into Azure:master Jan 24, 2018

lmazuel mentioned this pull request Jan 25, 2018

New rule: verify the parameters defined in the global parameters section Azure/azure-openapi-validator#84

Closed

Create a new version of consumption, with preview budget merge with usage detail and reservation #2289

Create a new version of consumption, with preview budget merge with usage detail and reservation #2289

Conversation

bgsky commented Jan 18, 2018 • edited Loading

PR information

Quality of Swagger

marstr commented Jan 19, 2018

bgsky commented Jan 19, 2018

marstr commented Jan 19, 2018

sandeepnl commented Jan 23, 2018

jhendrixMSFT left a comment

Choose a reason for hiding this comment

jhendrixMSFT commented Jan 23, 2018

AutorestCI commented Jan 23, 2018

bgsky commented Jan 23, 2018 • edited Loading

deprecatedAccount1 commented Jan 23, 2018

jhendrixMSFT commented Jan 23, 2018

bgsky commented Jan 23, 2018

sandeepnl commented Jan 23, 2018

jhendrixMSFT commented Jan 23, 2018

jhendrixMSFT Jan 23, 2018

Choose a reason for hiding this comment

jhendrixMSFT commented Jan 23, 2018

bgsky commented Jan 23, 2018

sandeepnl commented Jan 23, 2018

bgsky commented Jan 23, 2018

sandeepnl Jan 23, 2018

Choose a reason for hiding this comment

bgsky Jan 23, 2018

Choose a reason for hiding this comment

jhendrixMSFT commented Jan 23, 2018

dsgouda commented Jan 24, 2018

jhendrixMSFT commented Jan 24, 2018

dsgouda commented Jan 24, 2018 • edited Loading

bgsky commented Jan 24, 2018 • edited Loading

bgsky commented Jan 24, 2018

dsgouda commented Jan 24, 2018

azuresdkciprbot commented Jan 24, 2018

jhendrixMSFT commented Jan 24, 2018

jhendrixMSFT commented Jan 24, 2018

dsgouda commented Jan 24, 2018

jhendrixMSFT commented Jan 24, 2018

bgsky commented Jan 24, 2018 • edited Loading

jhendrixMSFT commented Jan 24, 2018

azuresdkciprbot commented Jan 24, 2018

jhendrixMSFT commented Jan 24, 2018

AutorestCI commented Jan 24, 2018

AutorestCI commented Jan 24, 2018

lmazuel commented Jan 25, 2018

jhendrixMSFT commented Jan 25, 2018

bgsky commented Jan 18, 2018 •

edited

Loading

bgsky commented Jan 23, 2018 •

edited

Loading

dsgouda commented Jan 24, 2018 •

edited

Loading

bgsky commented Jan 24, 2018 •

edited

Loading

bgsky commented Jan 24, 2018 •

edited

Loading