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.

Sign up for GitHub

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

Jump to bottom

[Hub Generated] Review request for Microsoft.HanaOnAzure to add version 2017-11-03-preview #5466

Merged
merged 1 commit into from
Mar 29, 2019
Merged

[Hub Generated] Review request for Microsoft.HanaOnAzure to add version 2017-11-03-preview #5466

merged 1 commit into from
Mar 29, 2019

Conversation

pabowers
Copy link
Contributor

@pabowers pabowers commented Mar 25, 2019 •
edited
Loading

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

Contribution checklist:

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@azuresdkci azuresdkci requested a review from dsgouda March 25, 2019 20:10
@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI AutorestCI mentioned this pull request Mar 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-js

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-js#1874

This was referenced Mar 25, 2019
Closed
Closed
@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#5049

@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-go

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

@AutorestCI AutorestCI mentioned this pull request Mar 25, 2019
Merged
@AutorestCI
Copy link

AutorestCI commented Mar 25, 2019 •
edited
Loading

Automation for azure-sdk-for-java

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

lagalbra
lagalbra reviewed Mar 25, 2019
View reviewed changes
...naonazure/resource-manager/Microsoft.HanaOnAzure/preview/2017-11-03-preview/hanaonazure.json Outdated Show resolved Hide resolved
...naonazure/resource-manager/Microsoft.HanaOnAzure/preview/2017-11-03-preview/hanaonazure.json Outdated
"$ref": "#/parameters/HanaInstanceNameParameter"
},
{
"$ref": "#/parameters/MonitoringParameter"
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't added non-path parameters to the Swagger spec before; is this how they're meant to be specified?

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 believe so because that is also the way that it is done for tags

@pabowers pabowers requested a review from lagalbra March 25, 2019 21:37
dsgouda
dsgouda suggested changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Please add a config file similar to this.

@dsgouda dsgouda added Changes Required WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Mar 25, 2019
@pabowers
Copy link
Contributor Author

@dsgouda we already have a config file for the project. Do I need another one for this small addition?

@dsgouda
Copy link
Contributor

dsgouda commented Mar 25, 2019

@pabowers whoops I missed that.
Needs an ARM review. Also, if possible, please clean up the commit history

@pabowers pabowers force-pushed the dev-hanaonazure-Microsoft.HanaOnAzure-2017-11-03-preview branch from 63387d3 to 034e18b Compare March 26, 2019 18:29
This was referenced Mar 26, 2019
Closed
Merged
@pabowers pabowers requested a review from dsgouda March 26, 2019 18:36
dsgouda
dsgouda suggested changes Mar 26, 2019
View reviewed changes
...naonazure/resource-manager/Microsoft.HanaOnAzure/preview/2017-11-03-preview/hanaonazure.json Outdated
"tags": [
"HanaOnAzure"
],
"operationId": "HanaInstances_Monitoring",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the Verb_Noun format for operation id. A better id here would be Monitor_HanaInstances

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That causes a warning with autorest, should I do it anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you share what the error is

Copy link
Contributor Author

@pabowers pabowers Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

WARNING (PostOperationIdContainsUrlVerb/R2066/SDKViolation): OperationId should contain the verb: 'monitoring' in:'Monitor_HanaInstances'. Consider updating the operationId
    - file:///Users/pagebowers/repos/azure-rest-api-specs/specification/hanaonazure/resource-manager/Microsoft.HanaOnAzure/preview/2017-11-03-preview/hanaonazure.json:293:16 ($.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.HanaOnAzure/hanaInstances/{hanaInstanceName}/monitoring"].post.operationId)

@dsgouda

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, you can name it as Monitoring_HanaInstances then
On a related note, in general ARM end points should actually look like get/put, etc

...naonazure/resource-manager/Microsoft.HanaOnAzure/preview/2017-11-03-preview/hanaonazure.json Show resolved Hide resolved
@pabowers pabowers force-pushed the dev-hanaonazure-Microsoft.HanaOnAzure-2017-11-03-preview branch from 034e18b to 688cc86 Compare March 26, 2019 19:41
@pabowers pabowers requested a review from dsgouda March 26, 2019 19:46
@pabowers pabowers force-pushed the dev-hanaonazure-Microsoft.HanaOnAzure-2017-11-03-preview branch from 688cc86 to 0106ae1 Compare March 26, 2019 21:38
@pabowers
Copy link
Contributor Author

@dsgouda I'm a little confused about what exactly I need to do at this point. Our API is in preview and I believe that it has had an ARM review. Do we need a new review for this addition? Thanks

@dsgouda dsgouda added the ARM-overdue ARM review has not occurred within the expected timeframe label Mar 29, 2019
dsgouda
dsgouda approved these changes Mar 29, 2019
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM

@dsgouda dsgouda merged commit 381e545 into Azure:master Mar 29, 2019
@AutorestCI AutorestCI mentioned this pull request Mar 29, 2019
Merged
@dsgouda
Copy link
Contributor

dsgouda commented Apr 16, 2019

@pabowers Apologize for the incorrect feedback earlier, HanaInstances_Monitor should be the correct operation id name. Please make the changes in a new PR and I'd be happy to review and merge the changes

@iscai-msft iscai-msft mentioned this pull request Apr 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsgouda dsgouda dsgouda approved these changes

@lagalbra lagalbra Awaiting requested review from lagalbra

Assignees

@dsgouda dsgouda

Labels
ARM-overdue ARM review has not occurred within the expected timeframe WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pabowers @azuresdkci @AutorestCI @dsgouda @lagalbra