Skip to content

Conversation

@hari-bodicherla
Copy link
Contributor

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Oct 20, 2018

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#295

@AutorestCI
Copy link

AutorestCI commented Oct 20, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Oct 20, 2018

Automation for azure-sdk-for-node

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

@AutorestCI
Copy link

AutorestCI commented Oct 20, 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#3099

Copy link
Member

@jhendrixMSFT jhendrixMSFT left a comment

Choose a reason for hiding this comment

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

There are model validation failures for this new operation that need to be fixed, please see the log for details.

"required": false,
"schema": {
"$ref": "#/definitions/VirtualMachineScaleSetVMInstanceIDs"
"$ref": "#/definitions/VirtualMachineScaleSetReimageParameters"
Copy link
Member

Choose a reason for hiding this comment

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

This introduces breaking changes in SDK codegen, is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, VirtualMachineScaleSetReimageParameters is the wrapper class around the VirtualMachineScaleSetVMInstanceIDs containing new inputs like osdisk, datadisk list of lun values and temp disk reiamge inputs for the customer. This needs to be added

@hari-bodicherla
Copy link
Contributor Author

Hi Joel, I had looked into validation errors but not having any clue why they are failing which are unrelated to my new changes. Could you please let me know if I can do something from my side to fix these

@jhendrixMSFT
Copy link
Member

If you do a search in the log for the example name, e.g. Reimage a Virtual Machine., you will find the relevant failures. Here's an example.
error : operationId: VirtualMachines_Reimage scenario: Reimage a Virtual Machine. source: request responseCode: ALL severity: 0 errorCode: OBJECT_ADDITIONAL_PROPERTIES errorDetails: code: OBJECT_ADDITIONAL_PROPERTIES params: - - properties - location message: 'Additional properties not allowed: properties,location' path: '' title: /definitions/VirtualMachineReimageParameters description: Paramaters for Reimaging Virtual Machine. position: line: 4917 column: 40 url: >- specification/compute/resource-manager/Microsoft.Compute/stable/2018-06-01/compute.json

@AutorestCI
Copy link

AutorestCI commented Oct 24, 2018

Automation for azure-sdk-for-ruby

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

"api-version": "2018-06-01",
"vmName": "myVMName",
"parameters": {
"location": "West US",
Copy link
Member

Choose a reason for hiding this comment

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

This isn't in the schema and is causing one of the model validation failures.

"api-version": "2018-10-01",
"vmName": "myVMName",
"parameters": {
"location": "West US",
Copy link
Member

Choose a reason for hiding this comment

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

This isn't in the schema and is causing one of the model validation failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Joel, is location parameter culprit?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I believe so. Note that you can run this validation locally to verify the fix before updating the PR, see the docs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Joel for the tool to validate locally. I had updated the examples and validated it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However I see there are other validation errors coming for others unrelated to mine

@jhendrixMSFT
Copy link
Member

@hari-bodicherla while it would be nice to clean up the other failures, since they weren't introduced in this PR it's not necessary. Let me know how you want to proceed.

@hari-bodicherla
Copy link
Contributor Author

Hi Joel, I had cleaned up the other failures too which are unrelated to my changes and sent new iteration . Thanks..

@jhendrixMSFT jhendrixMSFT merged commit a0adc09 into Azure:master Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants