Skip to content

[Hub Generated] Review request for Microsoft.Consumption to add version stable/2019-05-01#6620

Merged
yungezz merged 43 commits intoAzure:masterfrom
santoshsinha100:dev-consumption-Microsoft.Consumption-2019-05-01
Aug 6, 2019
Merged

[Hub Generated] Review request for Microsoft.Consumption to add version stable/2019-05-01#6620
yungezz merged 43 commits intoAzure:masterfrom
santoshsinha100:dev-consumption-Microsoft.Consumption-2019-05-01

Conversation

@santoshsinha100
Copy link
Copy Markdown
Contributor

@santoshsinha100 santoshsinha100 commented Jul 15, 2019

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

Updated scope for Usage detail list
syntax error fix
fixing syntax error
added scope parameter
scope added
added scope
added scope
added scope
added scope
added scope
added scope
added scope
json correction
syntax correction
added reservationRecommendation, RI Usage Summary and RI Usage details
syntax correction
bug fix
bug fix
bug fix
bug fix
bug fix
bug fix
bug fix
@santoshsinha100 santoshsinha100 requested a review from kjeur as a code owner July 15, 2019 08:22
…on/stable/2019-05-01/examples/ReservationSummariesDaily.json

Co-Authored-By: Nick Schonning <nschonni@gmail.com>
@santoshsinha100
Copy link
Copy Markdown
Contributor Author

Hi Nick,

Can you mark this item complete. I already addressed all your suggestions. We need this to roll out ASAP.

Thanks & Regards
Santosh

@nschonni
Copy link
Copy Markdown
Contributor

Sorry, I'm not an MS employee, but I think there is instructions over here if you need to escalate https://armwiki.azurewebsites.net/rp_onboarding/ResourceProviderOnboardingAPIRevieworkflow.html

@santoshsinha100 santoshsinha100 added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jul 18, 2019
@santoshsinha100
Copy link
Copy Markdown
Contributor Author

Can some one please approve this PR. This is active since last 4 to 5 days.

@ms-premp
Copy link
Copy Markdown
Contributor

We have preview version for usage details already merged in
#6575
We are moving this to GA and adding one more API in this GA version. Please review.

@majastrz
Copy link
Copy Markdown
Member

"/providers/Microsoft.Billing/billingAccounts/{billingAccountId}/providers/microsoft.consumption/ReservationRecommendations": {

nit - should be camelCase


Refers to: specification/consumption/resource-manager/Microsoft.Consumption/stable/2019-05-01/consumption.json:968 in d04e302. [](commit_id = d04e302, deletion_comment = False)

@majastrz
Copy link
Copy Markdown
Member

    "billingPeriodId": {

This is potentially an SDK breaking change. (When users update the current SDK to one generated from this Swagger, their code won't compile if they are referencing properties that got removed.) What's the plan to deal with that?


Refers to: specification/consumption/resource-manager/Microsoft.Consumption/stable/2019-05-01/consumption.json:1269 in 71e2dcb. [](commit_id = 71e2dcb, deletion_comment = True)

@majastrz majastrz added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required potential-sdk-breaking-change labels Jul 31, 2019
Copy link
Copy Markdown
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

Signed off from ARM side.

@santoshsinha100
Copy link
Copy Markdown
Contributor Author

Can some one do this review on priority. Its waiting for an approval

@majastrz
Copy link
Copy Markdown
Member

majastrz commented Aug 1, 2019

Can some one do this review on priority. Its waiting for an approval

I'm confused by this. Normally a person from the SDK team is assigned to a PR like this and they finish the review from the SDK side once ARM (me this week) signs off. I'll send an email to a contact I know internally (I don't know her GitHub name to just @ mention here.)

@majastrz
Copy link
Copy Markdown
Member

majastrz commented Aug 1, 2019

If you reach out to me directly via Teams (alias is same as my github name), I can add you to the mail thread.

Copy link
Copy Markdown
Member

@yungezz yungezz 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, some minor issue in samples. @santoshsinha100 could you pls resolve conflict? then I'll go to approve and merge the PR.

],
"excludedSubscriptions": [],
"usageStart": "2019-05-01T00:00:00.0000000Z",
"usageEnd": "2018-10-31T00:00:00.0000000Z",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

usageStart is later than usageEnd

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi I am unable to resolve the conflict in readme.md file. UI doesn't allow me to resolve. Looking at the conflict, I will go with "### Tag: package-2019-05" not the preview one. Will it be possible to get this resolved at your end .

"api-version": "2019-05-01",
"billingAccountId": "1234",
"departmentId": "42425",
"scope": "providers/Microsoft.Billing/BillingAccounts/1234",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor, indention

@yungezz yungezz self-assigned this Aug 2, 2019
@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation Bot commented Aug 5, 2019

In Testing, Please Ignore

[Logs] (Generated from 052f9db, Iteration 1)

Warning Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
  • Warning consumption/mgmt/2017-11-30 [Logs]
  • Warning consumption/mgmt/2018-01-31 [Logs]
  • Warning consumption/mgmt/2018-03-31 [Logs]
  • Warning consumption/mgmt/2018-05-31 [Logs]
  • Warning consumption/mgmt/2018-06-30 [Logs]
  • Warning consumption/mgmt/2018-08-31 [Logs]
  • Warning consumption/mgmt/2018-10-01 [Logs]
  • Warning consumption/mgmt/2019-01-01 [Logs]
  • Warning preview/consumption/mgmt/2017-04-24-preview [Logs]
  • Warning preview/consumption/mgmt/2017-12-30-preview [Logs]
Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Warning Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Warning consumption/resource-manager/v2017_04_24_preview [Logs]
  • Warning consumption/resource-manager/v2017_11_30 [Logs]
  • Warning consumption/resource-manager/v2018_01_31 [Logs]
  • Warning consumption/resource-manager/v2018_03_31 [Logs]
  • Warning consumption/resource-manager/v2018_05_31 [Logs]
  • Warning consumption/resource-manager/v2018_06_30 [Logs]
  • Warning consumption/resource-manager/v2018_08_31 [Logs]
  • Warning consumption/resource-manager/v2018_10_01 [Logs]
  • Warning consumption/resource-manager/v2019_01_01 [Logs]
Warning JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
  • Warning @azure/arm-consumption [Logs]
Warning Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]
  • No packages generated.

@yungezz
Copy link
Copy Markdown
Member

yungezz commented Aug 5, 2019

@santoshsinha100 pls look at the resolved readme.md file. pls update the tag package-preview-2019-05 to package-2019-05 at https://github.com/Azure/azure-rest-api-specs/pull/6620/files#diff-19989b3082cb3c89e63e9903a79a80b6R40 to

Copy link
Copy Markdown
Member

@yungezz yungezz left a comment

Choose a reason for hiding this comment

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

pls review change in readme.md for conflict resolving, and update tag name per need.

Copy link
Copy Markdown
Contributor

@ms-premp ms-premp left a comment

Choose a reason for hiding this comment

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

Thanks @yungezz. Your ReadMe changes looks good.

@yungezz yungezz merged commit 815d420 into Azure:master Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants