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

Adding various operations and examples for Budget APIs #2144

Merged
merged 12 commits into from
Dec 15, 2017
Merged

Adding various operations and examples for Budget APIs #2144

merged 12 commits into from
Dec 15, 2017

Conversation

asarkar84
Copy link
Contributor

@asarkar84 asarkar84 commented Dec 13, 2017

Adding various operations and examples for Budget APIs

  1. Operation to Get the List of Budgets for a subscription.
  2. Operation to Get the specific budget by name for a subscription.
  3. Operation to Create or Update a budget.
  4. Operation to Delete an existing budget.
  5. Examples for Create, Update, Delete, Get and List budgets.

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

Adding various operations for Budget APIs
…ined roles.

Updated the contact roles to be a free text field to support user defined roles.
Copy link
Member

@fearthecowboy fearthecowboy left a comment

Choose a reason for hiding this comment

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

Do not move the file common-types file; please reference it where it is.

ie, do not rename or copy the file :

/specification/common-types/resource-management/v1/types.json

"swagger": "2.0",
"info": {
"version": "2017-12-30-preview",
"title": "ConsumptionManagementClient",
Copy link
Member

Choose a reason for hiding this comment

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

title should just be "Consumption Management"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think last time when it was changed @lmazuel Suggested it this way due to the requirement on the SDK to generate the client name correctly.

Copy link
Member

Choose a reason for hiding this comment

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

@fearthecowboy Again, this is a C# behavior. Please move this "auto Client suffixing" to the modeler if you wish this behavior for everybody. In the meantime, changing that will break Python (at least, but probably not only)

Copy link
Member

@fearthecowboy fearthecowboy Dec 14, 2017

Choose a reason for hiding this comment

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

@lmazuel Right!

@sandeepnl - leave it for now, I gotta go fix the modeler/input first, and I'll have add a warning of the superflous suffix, so that in the future we can slowly clean them up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! We will leave it as is. Thanks!

"tags": [
"Budget"
],
"operationId": "Budget_Get",
Copy link
Member

Choose a reason for hiding this comment

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

the prefix on operationId is generally plural; so should be budgets_...

}
}
},
"Resource": {
Copy link
Member

Choose a reason for hiding this comment

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

Resource should be referenced from the common-types file, not copied

}
},
"x-ms-pageable": {
"nextLinkName": "null"
Copy link
Member

@fearthecowboy fearthecowboy Dec 13, 2017

Choose a reason for hiding this comment

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

So your nextLink Property Name is "null" ?

hint: if you have { "null", or "false", or "true" } in quotes, that's usually a bad thing 😆

Copy link
Member

@fearthecowboy fearthecowboy left a comment

Choose a reason for hiding this comment

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

I've added a few bits of feedback; I'll add more in a bit...

@fearthecowboy
Copy link
Member

Adding @ravbhatnagar for ARM review of new APIS

Updated with review comments.
Added required fields
Incorporated ARM review comments
Added min and max lengths for EmailContacts and Notifications
Renamed to ProxyResource
@asarkar84
Copy link
Contributor Author

asarkar84 commented Dec 14, 2017

I could not find the eTag property for ProxyResource in common-types. Shall I go ahead and add the eTag property there and refer it in this file?

@fearthecowboy
Copy link
Member

I could not find the eTag property for ProxyResource in common-types. Shall I go ahead and add the eTag property there and refer it in this file?

I just had this same discovery with another RP this morning.

For today, can you just add the etag property into your resource model(s) ; I think we have to have a second version of some of these (for older RPs that don't have all the fields).

We'll circle back once I can go bang on a few doors.

Copy link
Contributor

@sandeepnl sandeepnl left a comment

Choose a reason for hiding this comment

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

Let us rename this to "Budget-PREVIEW" as we want this name to show up in the Rest Documentation.

"/subscriptions/{subscriptionId}/providers/Microsoft.Consumption/budgets": {
"get": {
"tags": [
"Budgets"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let us rename this to "Budget-PREVIEW" as we want this name to show up in the Rest Documentation.
@fearthecowboy Please comment if this is not recommended as per the standards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I am going to rename the tag to Budgets-Preview @fearthecowboy Please let me know if this is OK.

@fearthecowboy
Copy link
Member

@fearthecowboy
Copy link
Member

Added Preview in Tag name to specify the Preview version
Incorporated review comments
Fixed validation errors
@azuresdkciprbot
Copy link

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.

Updated the example for CreateOrUpdateBudget
@azuresdkciprbot
Copy link

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.

@AutorestCI
Copy link

@AutorestCI
Copy link

mccleanp pushed a commit that referenced this pull request Mar 23, 2022
* update comment and fixed enum typo

* fixed indentation

* revert indentation change

* fixed indentation

Co-authored-by: Feng Wu <xuefwu@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants