Adding new properties to Consumption Usage Details #3357

ampravinr · 2018-07-05T19:59:06Z

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.

AutorestCI · 2018-07-05T19:59:44Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2158

AutorestCI · 2018-07-05T20:00:29Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

AutorestCI · 2018-07-05T20:01:19Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2700

ampravinr · 2018-07-05T20:02:00Z

@sandeepnl @asarkar84 @ms-premp Please review Usage Details changes

AutorestCI · 2018-07-05T20:04:18Z

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1479

AutorestCI · 2018-07-05T20:27:40Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2226

asarkar84

Looks good.

asarkar84 · 2018-07-05T22:21:43Z

...e-manager/Microsoft.Consumption/stable/2018-06-30/examples/UsageDetailsListByDepartment.json

@@ -0,0 +1,45 @@
+{
+  "parameters": {
+    "api-version": "2018-05-31",


Let's update the api version to 2018-06-30 in all examples.

annatisch · 2018-07-09T14:02:47Z

Thanks @ampravinr!
I think there might be an omission in the readme.md, as looking at the clients generated from this PR - many operations have been removed - is that intentional?
e.g. Python:
https://github.com/Azure/azure-sdk-for-python/pull/2877/files

ampravinr · 2018-07-09T18:18:52Z

@annatisch Thanks for the review. That is not intentional. Found one issue from my side and working on fixing the same.

… that those operations will be supported in the new version as well.

…e automatic system since we are manually generating the SDK for Python.

ampravinr · 2018-07-10T23:24:34Z

@annatisch I have made the changes (merged all required operations for the new version and removed the python sdk from the list of auto generated SDK generation - since Python SDK is manually generated). Please review and let me know if any issues.

sandeepnl · 2018-07-10T23:29:54Z

If i remember correctly python auto-gen was added to handle cases where there were minor non-breaking chganges which can be done without any human intervention.
@lmazuel Can you please confirm if removing python SDK auto-gen is okay?

ampravinr · 2018-07-11T20:16:41Z

@annatisch - I have made all the necessary changes. Could you please verify and let me know if any issues.

annatisch · 2018-07-11T20:41:22Z

Thanks @ampravinr - the diff is still showing the loss of some operations groups:
https://github.com/Azure/azure-sdk-for-python/pull/2877/files#diff-ab141b6de2e9066430c19456d8754671

Have these been removed or renamed?

ampravinr · 2018-07-11T22:14:42Z

@annatisch - I verified these changes and the latest changes are good to go.

None of these operations are present in our existing "azure-sdk-for-python/azure-mgmt-consumption/azure/mgmt/consumption/models/init.py" file. Also these operations are suppose to belongs to other namespace (cost-management). I believe the last commits - 7d850c0#diff-ebc1cee9670996e7d7037ed33bd69b9a and 30f1846#diff-ebc1cee9670996e7d7037ed33bd69b9a might have caused this.

We are good with the current changes since those which are missing is not supposed to be in there. Let me know if any issues.

annatisch · 2018-07-11T22:24:26Z

Thanks for the confirmation @ampravinr.
There seems to be a number of model validation errors in the examples:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/402794446

Could you take a look?

ampravinr · 2018-07-11T22:52:37Z

On it now. Thanks.

lmazuel · 2018-07-11T23:34:45Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-06-30/consumption.json

+        "tags": [
+          "Balances"
+        ],
+        "operationId": "GetBalancesByBillingAccount",


You cannot have at the same time:

GetBalancesByBillingAccount_ByBillingPeriod

GetBalancesByBillingAccount

Since the syntax XXX_YYY means for Autorest "Creates an operation group called XXX with operation YYY", while the syntax XXX means create "XXX operation". So there is conflict, if GetBalancesByBillingAccount means functions or operation group.

Seeing the url, I would instinctively do:

Balances_GetByBillingAccount

Balances_GetByBillingPeriod

ok I got it. I will make the changes. Thanks a lot @lmazuel

ampravinr · 2018-07-12T01:59:50Z

@annatisch , @lmazuel, @sandeepnl - I have made the necessary changes. Could you please review and let me know if any issue. I see errors in #10878.14 and #10878.15 which I think is not because of our changes. Please let me know if its something I need to fix. Thanks.

lmazuel · 2018-07-12T16:09:50Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-06-30/consumption.json

+          }
+        },
+        "x-ms-pageable": {
+          "nextLink


You have both at the same time a model called ReservationRecommendations and an operation ID called ReservationRecommendations.
Usually, the operation is the plural form and the model the singular form: Operation ID ReservationRecommendations has operations that returns list of ReservationRecommendation

Is this true that this model "ReservationRecommendations" is actually ONE recommendation. If yes, please rename it "ReservationRecommendation" (and ReservationRecommendationProperties and ReservationRecommendationListResult)

Having the same string for Operation group ID and a model might (depending of the language) create troubles.

Sure, got it. Let me take a look and will change it accordingly. Thanks again.

lmazuel

One final comment you may address if it makes sense for your service.

lmazuel · 2018-07-12T16:26:15Z

@ampravinr Just to triple-check, with this new api version I see that "report_config/billing_account_dimensions/subscription_dimensions/resource_group_dimensions" no longer exists in the final SDK. Could you confirm it's on purpose?

ampravinr · 2018-07-12T16:42:57Z

@lmazuel - I verified the same and we are good here. ResourceGroupDimensions_List operation is moved from Consumption to another namespace\swagger. The same is moved to - "\azure-rest-api-specs\specification\cost-management\resource-manager\Microsoft.CostManagement\stable\2018-05-31\costmanagement.json" now.

…ry), ReservationDetails (now: ReservationDetail) and ReservationRecommendations (now: ReservationRecommendation)

ampravinr · 2018-07-12T18:46:00Z

@annatisch - I have made the required changes and not seeing any errors from my side. Please review and let me know if any issues or if any concerns.

annatisch · 2018-07-12T19:09:02Z

Thanks @ampravinr - a couple of things:

Why did you rename CostTags to CostTagsResult? This model is actually passed in as a request body for the operation CostTags_CreateOrUpdate so calling it "result" is doesn't quite fit :)
Please remove the final 's' from the ends of these operations:
- "Marketplaces_ListByEnrollmentAccounts"
- "Marketplaces_ListForBillingPeriodByEnrollmentAccounts"
I would like to clarify the different between the operations:
- budgets_get
- budgets_getByResourceGroupName
Does this mean that the latter operation will retrieve only the budget for the specified resource group? If so the naming is a bit confusing because the meaning is a bit different then budgets_listByResourceGroup. Maybe this could be better clarified by naming like so:
- budets_get
- ResourceGroupBudgets_get (and the same for the equivalent operations, like ResourceGroupBudgets_delete)
Unless I've misunderstood what these APIs do?

Thanks!

Thanks @lmazuel for taking a look :)

ampravinr · 2018-07-12T19:34:18Z

@annatisch - please find below:

changed CostTags to CostTagsResult because we have operations with CostTags like CostTags_Get and according to the errors as well as per @lmazuel that is not a good practice to have it like that.
I will update the operation id to CostTag and will revert the CostTagsResult to CostTags if that is acceptable.
For the Budgets, the later is specific to Resource group. We would like to keep the same naming since all our other operations also follow the same naming. For example - UsageDetails_ListByBillingAccount, UsageDetails_ListByDepartment etc. So we would like to keep the same here. Also this is something from the exsting version (03-31) and we don't have changes in name for these operations.
Removing the 's' from ends of these operations "Marketplaces_ListByEnrollmentAccounts" and
"Marketplaces_ListForBillingPeriodByEnrollmentAccounts"

…nged operations - Marketplaces_ListByEnrollmentAccount and Marketplaces_ListForBillingPeriodByEnrollmentAccount.

ampravinr · 2018-07-12T19:57:19Z

Hi @annatisch, I have made the above changes. Please review and let me know your thoughts. (I have reverted CostTagsResult to CostTag and the operations are now CostTags_*. Let me know if any issues)

ampravinr · 2018-07-12T23:12:27Z

Thanks @annatisch , @lmazuel , @sandeepnl for all the comments and help.

lmazuel · 2018-07-12T23:27:08Z

@ampravinr About Python, you can fork this branch to save you Autorest call:
Azure/azure-sdk-for-python#2158

ampravinr added 2 commits July 5, 2018 10:22

Usage details V3 with location, service tier and service name

16a1362

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

4394e3b

AutorestCI mentioned this pull request Jul 5, 2018

[AutoPR consumption/resource-manager] Adding new properties to Consumption Usage Details Azure/azure-sdk-for-python#2877

Merged

azuresdkci assigned annatisch Jul 5, 2018

azuresdkci requested a review from annatisch July 5, 2018 20:00

AutorestCI mentioned this pull request Jul 5, 2018

[AutoPR consumption/resource-manager] Adding new properties to Consumption Usage Details Azure/azure-sdk-for-node#3120

Merged

AutorestCI mentioned this pull request Jul 5, 2018

[AutoPR consumption/resource-manager] Adding new properties to Consumption Usage Details Azure/azure-sdk-for-go#2189

Merged

Fixed the API versions for the examples.

6ddddfc

asarkar84 approved these changes Jul 5, 2018

View reviewed changes

annatisch added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jul 5, 2018

ampravinr added 2 commits July 5, 2018 22:24

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

142dcc1

Added new version settings details for Java to the ReadMe file

3e43230

ravbhatnagar 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 labels Jul 9, 2018

ampravinr added 4 commits July 9, 2018 11:34

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

8b45200

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

5620f27

Included all the previous operations and examples in the changeset so…

861b130

… that those operations will be supported in the new version as well.

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

8d551a2

AutorestCI mentioned this pull request Jul 10, 2018

[AutoPR consumption/resource-manager] Adding new properties to Consumption Usage Details Azure/azure-sdk-for-ruby#1465

Merged

Removing Python from the list of SDKs which should be generated by th…

5a5118c

…e automatic system since we are manually generating the SDK for Python.

lmazuel reviewed Jul 11, 2018

View reviewed changes

ampravinr added 4 commits July 11, 2018 16:59

Added missing properties merged from both 03/2018 and 05/2018

d1ba04a

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

bac7ebe

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

4c944a7

Modified the Operation Id to follow the best practices.

891beec

lmazuel reviewed Jul 12, 2018

View reviewed changes

lmazuel approved these changes Jul 12, 2018

View reviewed changes

ampravinr added 3 commits July 12, 2018 10:13

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

09d8ca0

Fixed model namings for - ReservationSummaries (now: ReservationSumma…

b9b62fd

…ry), ReservationDetails (now: ReservationDetail) and ReservationRecommendations (now: ReservationRecommendation)

Fixed the naming for model CostTags.

5d36df6

lmazuel approved these changes Jul 12, 2018

View reviewed changes

Fixing the SDKViolation error on - model Tags.

c5a8c67

ampravinr added 2 commits July 12, 2018 12:48

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

8db0ff5

Changed the CostTagResult to CostTag as per review comments. Also cha…

b28e418

…nged operations - Marketplaces_ListByEnrollmentAccount and Marketplaces_ListForBillingPeriodByEnrollmentAccount.

annatisch approved these changes Jul 12, 2018

View reviewed changes

annatisch merged commit db4a3b3 into Azure:master Jul 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new properties to Consumption Usage Details #3357

Adding new properties to Consumption Usage Details #3357

ampravinr commented Jul 5, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

ampravinr commented Jul 5, 2018

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

asarkar84 left a comment

asarkar84 Jul 5, 2018

annatisch commented Jul 9, 2018

ampravinr commented Jul 9, 2018

ampravinr commented Jul 10, 2018

sandeepnl commented Jul 10, 2018

ampravinr commented Jul 11, 2018

annatisch commented Jul 11, 2018

ampravinr commented Jul 11, 2018

annatisch commented Jul 11, 2018

ampravinr commented Jul 11, 2018

lmazuel Jul 11, 2018 •

edited

Loading

ampravinr Jul 11, 2018

ampravinr commented Jul 12, 2018

lmazuel Jul 12, 2018

ampravinr Jul 12, 2018

lmazuel left a comment

lmazuel commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

annatisch commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

lmazuel commented Jul 12, 2018

Adding new properties to Consumption Usage Details #3357

Adding new properties to Consumption Usage Details #3357

Conversation

ampravinr commented Jul 5, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-java

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-node

ampravinr commented Jul 5, 2018

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-go

asarkar84 left a comment

Choose a reason for hiding this comment

asarkar84 Jul 5, 2018

Choose a reason for hiding this comment

annatisch commented Jul 9, 2018

ampravinr commented Jul 9, 2018

ampravinr commented Jul 10, 2018

sandeepnl commented Jul 10, 2018

ampravinr commented Jul 11, 2018

annatisch commented Jul 11, 2018

ampravinr commented Jul 11, 2018

annatisch commented Jul 11, 2018

ampravinr commented Jul 11, 2018

lmazuel Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

ampravinr Jul 11, 2018

Choose a reason for hiding this comment

ampravinr commented Jul 12, 2018

lmazuel Jul 12, 2018

Choose a reason for hiding this comment

ampravinr Jul 12, 2018

Choose a reason for hiding this comment

lmazuel left a comment

Choose a reason for hiding this comment

lmazuel commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

annatisch commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

ampravinr commented Jul 12, 2018

lmazuel commented Jul 12, 2018

ampravinr commented Jul 5, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading

lmazuel Jul 11, 2018 •

edited

Loading