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

Renamed cost allocation tags to cost tags and added schema for get list of all available tag keys #2862

Merged
merged 28 commits into from
Apr 13, 2018

Conversation

asarkar84
Copy link
Contributor

@asarkar84 asarkar84 commented Apr 12, 2018

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

asarkar84 and others added 25 commits March 8, 2018 15:44
Added Tags filter for budgets and updated the api version
Updated comments
Incorporated review comments
Incorporated review comments
Incorporated review comment
Added Tags filter and grouping for UsageDetails
Added reservation recommendations and tags
Incorporated review comments
Incorporated review comments
Removed unwanted space
Removed extra whitespace
Added schema spec for cost allocation tags
Updated Get Operation
Incorporated review comments
Review comments incorporated
Updated path
…st of all available tag keys

Renamed cost allocation tags to cost tags and added schema for get list of all available tag keys
@AutorestCI
Copy link

AutorestCI commented Apr 12, 2018

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

@AutorestCI
Copy link

AutorestCI commented Apr 12, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Apr 12, 2018

Automation for azure-sdk-for-node

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

@AutorestCI
Copy link

AutorestCI commented Apr 12, 2018

Automation for azure-sdk-for-go

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-go#1612

@asarkar84
Copy link
Contributor Author

@jianghaolu Please take a look. Thanks!

@@ -1868,6 +1907,43 @@
}
}
},
"Tags": {
"description": "A tag resource.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put in more detailed descriptions. This is easily confused with "Tag".

Copy link
Contributor

Choose a reason for hiding this comment

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

You might even consider renaming this to "TagResource".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the description. Please take a look if the changes are fine.

"type": "Microsoft.Consumption/tags",
"eTag": "\"1d34d012214157f\"",
"properties": {
"tags": [
Copy link
Contributor

Choose a reason for hiding this comment

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

The property name here should be "costTags", as defined in TagProperties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! for mentioning this. Actually it was wrong at the other place. I am corrected it there.

Incorporated review comments
Incorporated review comments
"description": "The properties of the tag.",
"properties": {
"tags": {
"description": "Tags.",
Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing : Please avoid using property name as the description. Try to add a little more info :)

Copy link
Contributor Author

@asarkar84 asarkar84 Apr 12, 2018

Choose a reason for hiding this comment

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

Sure! I have made the changes. Will keep this in mind for future as well.

Incorporated review comments
@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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@jianghaolu
Copy link
Contributor

Per offline discussion - this is a breaking change but 4/13/18 is when the service is first deployed. So this is accepted for now and no future breaking changes will be allowed to this spec.

@jianghaolu jianghaolu merged commit b9da36a into Azure:master Apr 13, 2018
@jhendrixMSFT
Copy link
Member

@AutorestCI regenerate azure-sdk-for-go

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.

5 participants