Skip to content

Updating documentation for Microsoft.VisualStudio with project resour…#1498

Merged
balajikris merged 1 commit intoAzure:currentfrom
niblak:current
Aug 8, 2017
Merged

Updating documentation for Microsoft.VisualStudio with project resour…#1498
balajikris merged 1 commit intoAzure:currentfrom
niblak:current

Conversation

@niblak
Copy link
Copy Markdown
Contributor

@niblak niblak commented Aug 1, 2017

…ce APIs. Corrections to existing examples.

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

@msftclas
Copy link
Copy Markdown

msftclas commented Aug 1, 2017

@niblak,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how best to express the fact that these two properties are required (ProcessTemplateId and VersionControlOption).

Also, there's a relatively static range of values that are accepted for these. If they were formal parameters I'd consider using x-ms-enum in the RP spec to list all possible values, but in this particular case I am not sure how to express that in documentation.

Copy link
Copy Markdown
Contributor

@balajikris balajikris Aug 3, 2017

Choose a reason for hiding this comment

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

I made a comment about this here. I think that should work, but you could quickly verify that by generating C# library using autorest and viewing the definition for ProjectResource

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll give that a try.

Is it possible to use the enum semantics on non-string types? ProcessTemplateId is actually a GUID, and the only accepted values are 6B724908-EF14-45CF-84F8-768B5384DA45, ADCC42AB-9882-485E-A3ED-7678F01F66BC, 27450541-8E31-4150-9947-DC59F998FC01.

From looking at other RP specs and the Swagger docs, I don't see a way to assign a friendly name to these (the IDs correspond to Scrum, Agile and Cmmi process templates).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you are correct that there isn't a way to do this as of today. For the future, I hear that there is a proposal to expand the scope of x-ms-enum to accommodate this kind of friendly name assignment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you could model this like so:

       "properties": {
         "properties": {
           "ProcessTemplateId": { "required": true },
           "VersionControlOption": { "required": true },
           "properties": {
               "type": "object",
    	       "additionalProperties": {
			"type": "string"
		 },
		 "x-ms-client-flatten": true
           },
          "x-ms-client-flatten": true
         }
       }

@balajikris
Copy link
Copy Markdown
Contributor

@niblak - The CI job found some errors with the new examples you added for the operations that are being added by this PR. See here
If you scroll all the way to the end, you can see that the last 4-5 failing examples were the ones you added. Could you please update your sample to make validation pass?

@niblak niblak force-pushed the current branch 6 times, most recently from 725cfd9 to 232efa5 Compare August 8, 2017 17:56
@balajikris
Copy link
Copy Markdown
Contributor

@niblak -- I can see you are working on this, can you please tag me in a comment after you are done fixing the validation errors? Thanks.

@niblak
Copy link
Copy Markdown
Contributor Author

niblak commented Aug 8, 2017

@balajikris sorry about all the pushes.

I've resolved most of the example errors but am confused on the remaining two: INVALID_RESPONSE_BODY and INVALID_REQUEST_PARAMETER.

What am I missing here? It seems to be complaining about properties that are actually part of the response/request body for our API.

Latest PR build here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be removed

@balajikris
Copy link
Copy Markdown
Contributor

Hey @niblak -

Sure, Its complaining because you have plan as an input to your updateproject call but the schema doesn't say anything about plan being an input to the api call. This is probably a result of copy pasting from another example? For e.g. see /UpdateExtensionResource.json example, plan is one of the inputs but in this case, the spec defines it as one of the input params.

@niblak niblak force-pushed the current branch 2 times, most recently from a49edda to 07ff683 Compare August 8, 2017 20:49
@azuresdkciprbot
Copy link
Copy Markdown

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/visualstudio/resource-manager/readme.md
Before the PR: Warning(s): 4 Error(s): 0
After the PR: Warning(s): 6 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

@niblak
Copy link
Copy Markdown
Contributor Author

niblak commented Aug 8, 2017

Thanks @balajikris. That was the problem and the CI build is passing now. Build #10 is failing due to a null ref, should I be concerned about this one?

The syntax for specifying ProcessTemplateId and VersionControlOption as required didn't seem to work unfortunately (failed both the PR build and C# client generation). For now, I've just added the list of valid values to the method summary. Not ideal, but there seems to be precedent for this in other RP specs.

@azuresdkciprbot
Copy link
Copy Markdown

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/visualstudio/resource-manager/readme.md
Before the PR: Warning(s): 4 Error(s): 0
After the PR: Warning(s): 6 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

@balajikris
Copy link
Copy Markdown
Contributor

@niblak - don't worry about the null ref on the build. Seems like a bug in the tool. CI looks much nicer now, thanks for making hte changes 👍 Will review again in a bit

@balajikris
Copy link
Copy Markdown
Contributor

@niblak - If you are all done here, I'm ready to pull this in. Let me know. Thanks.

@niblak
Copy link
Copy Markdown
Contributor Author

niblak commented Aug 8, 2017

@balajikris Good to go. Thanks for the review!

@balajikris balajikris merged commit 45f3b8b into Azure:current Aug 8, 2017
@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-node

@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-ruby

@AutorestCI
Copy link
Copy Markdown

No modification for AutorestCI/azure-sdk-for-python

anuchandy pushed a commit that referenced this pull request Aug 14, 2017
…d balancer (#1528)

* Updating documentation for Microsoft.VisualStudio with project resource APIs. Corrections to existing examples. (#1498)

* Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs (#1502)

* Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs

* Updated tag names

* Microsoft.Sql - Added data sync to package-2015-05-preview (#1510)

* Added advisors and data sync to package-2015-05-preview

* Removed advisors so current branch is only data sync

* Make MsDeploy and MsDeployLog Azure resources (#1519)

* Add public certificates, Functions Admin Token and MSDeploy APIs

* Revert resource definition changes and description change for site properties

* Make type and name readonly properties. Add ARM envelope to MSDeployStatus object

* Fix missing quotation issue.

* Add long running operation to MS deploy

* Make MsDeploy and MsDeployLog Azure resources

* Fix some AutoRest validation issues. Use dictionary for msdeploy.setparameters

* Ensure models are same across all schemas in Microsoft.Web

* Fix azure resource type across all models

* Remove conflicting ListOperations. It is not used for public Azure.

* no YAML in glob patterns for report scripts

* Copying 2017-06-01 to 2017-08-01

* Changes for new API version

* Updated script to run autorest with grouping swaggers based on config file (#1529)

* Removed the maxLength for entities (#1527)

* Update folder structure example. (#1489)

Agreed on the changes

* [Travis-ci] Oad in ci status (#1520)

* Breaking change build should fail if result contains error
Skipping newly added files
Incorporating review feedback

* testing

* log the result as oad.compare returns promise for caller

* Use published oad

* Revert "testing"

This reverts commit a9be94d.

* Removed maxLength from entity name (#1531)

* This is to add Cognitive Services data-plane API specs starting with Face API. (#1467)

* Adding data plane spec starting with Face

* Move Spec to right folder

* Fixing validation issues.

* Fixing issues and adding a readme.md

* Fixing examples by removing decimals.

* Removing another decimal value

* Update readme.md

* Updating spec based on comments.

* Removing .gitignore.

* Relay: Removed the maxLength for entity names (#1535)
anuchandy pushed a commit that referenced this pull request Aug 15, 2017
…d balancer (#1528) (#1543)

* Updating documentation for Microsoft.VisualStudio with project resource APIs. Corrections to existing examples. (#1498)

* Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs (#1502)

* Added Api-Version 2017-07-01 for RecoveryServices.Backup Jobs

* Updated tag names

* Microsoft.Sql - Added data sync to package-2015-05-preview (#1510)

* Added advisors and data sync to package-2015-05-preview

* Removed advisors so current branch is only data sync

* Make MsDeploy and MsDeployLog Azure resources (#1519)

* Add public certificates, Functions Admin Token and MSDeploy APIs

* Revert resource definition changes and description change for site properties

* Make type and name readonly properties. Add ARM envelope to MSDeployStatus object

* Fix missing quotation issue.

* Add long running operation to MS deploy

* Make MsDeploy and MsDeployLog Azure resources

* Fix some AutoRest validation issues. Use dictionary for msdeploy.setparameters

* Ensure models are same across all schemas in Microsoft.Web

* Fix azure resource type across all models

* Remove conflicting ListOperations. It is not used for public Azure.

* no YAML in glob patterns for report scripts

* Copying 2017-06-01 to 2017-08-01

* Changes for new API version

* Updated script to run autorest with grouping swaggers based on config file (#1529)

* Removed the maxLength for entities (#1527)

* Update folder structure example. (#1489)

Agreed on the changes

* [Travis-ci] Oad in ci status (#1520)

* Breaking change build should fail if result contains error
Skipping newly added files
Incorporating review feedback

* testing

* log the result as oad.compare returns promise for caller

* Use published oad

* Revert "testing"

This reverts commit a9be94d.

* Removed maxLength from entity name (#1531)

* This is to add Cognitive Services data-plane API specs starting with Face API. (#1467)

* Adding data plane spec starting with Face

* Move Spec to right folder

* Fixing validation issues.

* Fixing issues and adding a readme.md

* Fixing examples by removing decimals.

* Removing another decimal value

* Update readme.md

* Updating spec based on comments.

* Removing .gitignore.

* Relay: Removed the maxLength for entity names (#1535)
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