Skip to content

Conversation

@vrdmr
Copy link
Member

@vrdmr vrdmr commented Feb 3, 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 has 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

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-go:
Azure/azure-sdk-for-go@d3f1eca

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-go:
Azure/azure-sdk-for-go@f19be4f

@fearthecowboy
Copy link
Contributor

Hey @vrdmr ,

I was about to start reviewing the PR, but it still has an awful lot of validation errors.

See: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/337677030

Can you review those, and see what you can do to get it to pass?

@vrdmr
Copy link
Member Author

vrdmr commented Feb 6, 2018

Hi @fearthecowboy,

Thanks. I'll take a look.

I am trying to run the model-validator but I am hitting the following:

varads-mbp:resource-manager varadmeru$ autorest --model-validator readme.md 
AutoRest code generation utility [version: 2.0.4245; node: v9.4.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/private/var/folders/sp/dpmjjqvj7gs3ylxw_08wpxk40000gn/T/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4245)
   Loading local AutoRest extension 'oav' (/usr/local/Cellar/node/9.4.0/lib/node_modules/oav)
FATAL: TypeError: Cannot read property 'ReadFile' of undefined
FATAL: swagger-document/model-validator - FAILED
FATAL: Error: Plugin model-validator reported failure.
Process() cancelled due to exception : Plugin model-validator reported failure.
  Error: Plugin model-validator reported failure.
varads-mbp:resource-manager varadmeru$ 

and when using the --validation and --oav options, I do not see any validation issues.

varads-mbp:resource-manager varadmeru$ autorest --validation --oav
AutoRest code generation utility [version: 2.0.4245; node: v9.4.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/private/var/folders/sp/dpmjjqvj7gs3ylxw_08wpxk40000gn/T/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4245)
varads-mbp:resource-manager varadmeru$
varads-mbp:resource-manager varadmeru$ autorest --oav
AutoRest code generation utility [version: 2.0.4245; node: v9.4.0]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/private/var/folders/sp/dpmjjqvj7gs3ylxw_08wpxk40000gn/T/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4245)
varads-mbp:resource-manager varadmeru$ 

Could you please help me with this.

Thanks,
Varad

@fearthecowboy
Copy link
Contributor

Hey @vrdmr ,

I'm seeing the problem using --model-validator too. I'm looking into it.

In the mean time, you can use the model-validator outside of autorest on individual swagger files:

Example

# ensure the module globally
> npm install -g oav

# example: run it against a swagger file directly.
> oav validate-example Microsoft.Automation/stable/2015-10-31/dscCompilationJob.json

I'm looking into why using it as a plugin isn't working right now.

@fearthecowboy
Copy link
Contributor

FYI @vrdmr

This was the correct syntax (even tho it fails)

  
autorest --model-validator readme.md 

Using --oav and --validation don't do anything; the validators are activated with --model-validator and --azure-validator (you'll know which are correct, they will show you Loading autorest extension messages)

@fearthecowboy
Copy link
Contributor

Hey @vrdmr,

I've asked the OAV author to fix the autorest extension, I think we've got a few glitches he has to take care of.

in the mean time, fall back to using oav manually on each of the files.

@AutorestCI
Copy link

AutorestCI commented Feb 7, 2018

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#1928

@AutorestCI
Copy link

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: ['/usr/local/bin/autorest', '/tmp/tmpauiawjcr/rest/specification/automation/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmpauiawjcr/sdk', '--multiapi', '--package-version=v12.2.1-beta', '[email protected]/autorest.go@preview', "--user-agent='Azure-SDK-For-Go/v12.2.1-beta services'", '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4244; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest

There is a new version of AutoRest available (2.0.4245).
 > You can install the newer version with with npm install -g autorest@latest

   Loading AutoRest core      '/tmp/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4244)
   Loading AutoRest extension '@microsoft.azure/autorest.go' (preview->2.1.70)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2015-10"} .
ERROR: Schema violation: Data does not match any schemas from 'oneOf'
    - file:///tmp/tmpauiawjcr/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/dscConfiguration.json:256:10 ($.paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Automation/automationAccounts/{automationAccountName}/configurations/{configurationName}/content"].get.responses["200"])
FATAL: swagger-document/individual/schema-validator - FAILED
FATAL: Error: [OperationAbortedException] Error occurred. Exiting.
Process() cancelled due to exception : [OperationAbortedException] Error occurred. Exiting.
Failure during batch task - {"tag":"package-2015-10"} -- false.

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-go:
Azure/azure-sdk-for-go@78f84aa

@vrdmr
Copy link
Member Author

vrdmr commented Feb 7, 2018

Hi Garrett,

I have fixed most of the DSC related issues and am working with other teams to fix their resources specs. Could you please take a look at this PR now and merge it in when you get a chance?

Thanks,
Varad

@fearthecowboy

@Azure Azure deleted a comment from AutorestCI Feb 7, 2018
@Azure Azure deleted a comment from AutorestCI Feb 7, 2018
@Azure Azure deleted a comment from AutorestCI Feb 7, 2018
Copy link
Contributor

@fearthecowboy fearthecowboy Feb 8, 2018

Choose a reason for hiding this comment

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

Ouch!
You're changing enum values here.

Did the service change, or was this an error in the spec before? If this is a service change, we're potentially breaking an API.

As it is, this means that current clients are incorrect. Is this so?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Spec was incorrect here and I just changed it to reflect what we are sending from the service. And we are in the process of releasing new Automation SDKs based on Swagger and are still in preview mode.

Our last stable release of SDK was based on Hyak.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this allOf been removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This resource is a ProxyResource and does not have all the requirements of the Resource (like Location, Tags etc.). Thus I removed the allOf - Resource and just added the tag.

To make it better, I can go and take this route as mentioned here - #2368 (comment). By creating a separate Proxy Resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

And this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Explained above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this adding a new API or documenting an existing one that wasn't listed before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Documenting existing API which wasn't listed earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this deleted? This may result in a breaking change in the generated clients.

Copy link
Member Author

Choose a reason for hiding this comment

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

It returns only 200 and not 201. As mentioned earlier, we don't have any swagger-based stable clients and are in the process of releasing them.

I'll confirm the response code with Fiddler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, I checked with the team and I resolved the issue. They had added new flags and it wasn't documented in the spec, which caused the validation failures in DSCCompilationJob. Fixed it.

@fearthecowboy
Copy link
Contributor

Hey @vrdmr -- we're getting closer. Just a few things i need you to look at.

It's a bit tricky, since there are other validation failures and examples failing -- is it possible that whoever owns the examples that are failing could look at those and update this PR?

@AutorestCI
Copy link

AutorestCI commented Feb 12, 2018

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#1078

@vrdmr
Copy link
Member Author

vrdmr commented Feb 12, 2018

@fearthecowboy Hi Garrett, Could you please have a look at this PR. If you have any questions, we can have a quick sync. Thanks.

@fearthecowboy
Copy link
Contributor

@vrdmr - I'll get back to it first thing in the morning.

@fearthecowboy
Copy link
Contributor

@vrdmr -- You're still showing validator problems:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/340682526 -- invalid examples validation
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/340682523 -- a lot of these are flagging because they are marked as azure resources but not showing the required fields.

@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/automation/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 36
After the PR: Warning(s): 0 Error(s): 22

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@vrdmr
Copy link
Member Author

vrdmr commented Feb 13, 2018

Hi @fearthecowboy, For the issues you specified in the build, related to linting issues, we have active work going on to get them down to 0. There are actively three PRs (#2360, #2466 and this one) from our team and I would be sending one more PR for DSC issues.

For the first set of issues related to x-ms-example, I will push in the changes.

@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/automation/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 36
After the PR: Warning(s): 0 Error(s): 24

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@fearthecowboy
Copy link
Contributor

Merging this -- noting that other PRs (#2360, #2466) are intended to address some of the linter issues.

@fearthecowboy fearthecowboy merged commit b8c8d8b into Azure:master Feb 14, 2018
@vrdmr vrdmr deleted the vameru-dsc-swagger-validation-issues branch February 14, 2018 19:52
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