Skip to content

Rename 'options' parameters. Add config snapshots API. Add valid values for SCMType. #878

Merged
veronicagg merged 11 commits intoAzure:masterfrom
naveedaz:master
Jan 25, 2017
Merged

Rename 'options' parameters. Add config snapshots API. Add valid values for SCMType. #878
veronicagg merged 11 commits intoAzure:masterfrom
naveedaz:master

Conversation

@naveedaz
Copy link
Copy Markdown
Contributor

@naveedaz naveedaz commented Jan 23, 2017 •
edited by azuresdkci
Loading

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

  • I have read the contribution guidelines.
  • My spec meets the review criteria:
    • The spec conforms to the Swagger 2.0 specification.
    • Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
    • The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

veronicagg
veronicagg reviewed Jan 24, 2017
View reviewed changes
arm-web/2016-08-01/WebApps.json Outdated
"tags": [
"WebApps"
],
"summary": "Gets a snapshot of the configuration of an app, such as platform version and bitness, default documents, virtual applications, Always On, etc.",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

summary looks inaccurate for this operation, please review

Copy link
Copy Markdown
Contributor Author

@naveedaz naveedaz Jan 24, 2017

Choose a reason for hiding this comment

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

@Nking92 Please take a look.

Copy link
Copy Markdown

@nking-1 nking-1 Jan 24, 2017

Choose a reason for hiding this comment

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

I'll change this one to "Reverts the configuration of an app to a previous snapshot"

veronicagg
veronicagg reviewed Jan 24, 2017
View reviewed changes
arm-web/2016-08-01/WebApps.json Outdated
"WebApps"
],
"summary": "Gets a snapshot of the configuration of an app, such as platform version and bitness, default documents, virtual applications, Always On, etc.",
"description": "Gets a snapshot of the configuration of an app, such as platform version and bitness, default documents, virtual applications, Always On, etc.",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

description looks inaccurate for this operation

Copy link
Copy Markdown
Contributor Author

@naveedaz naveedaz Jan 24, 2017

Choose a reason for hiding this comment

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

@Nking92 Please take a look.

Copy link
Copy Markdown

@nking-1 nking-1 Jan 24, 2017

Choose a reason for hiding this comment

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

I think the description is accurate, but maybe it's a little confusing. You do get an entire site configuration back. I can change the first part of the sentence to "Gets the configuration of an app at a previous point in time" if that's more clear

veronicagg
veronicagg suggested changes Jan 25, 2017
View reviewed changes
arm-web/2016-08-01/WebApps.json
},
{
"name": "options",
"name": "migrationOptions",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

property name changes can cause breaking changes in the sdks, please review and confirm these changes are necessary and aligned with the service.

Copy link
Copy Markdown
Contributor Author

@naveedaz naveedaz Jan 25, 2017

Choose a reason for hiding this comment

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

This API is fairly new and does not exist in SDKs yet.

arm-web/2016-08-01/WebApps.json
"tags": [
"WebApps"
],
"summary": "Gets a list of web app configuration snapshots identifiers. Each element of the list contains a timestamp and the ID of the snapshot.",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

is this summary accurate? it doesn't mention slots

arm-web/2016-08-01/WebApps.json Outdated
"tags": [
"WebApps"
],
"summary": "Gets a snapshot of the configuration of an app, such as platform version and bitness, default documents, virtual applications, Always On, etc.",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

please review summary and description

arm-web/2016-08-01/WebApps.json Outdated
"tags": [
"WebApps"
],
"summary": "Gets a snapshot of the configuration of an app, such as platform version and bitness, default documents, virtual applications, Always On, etc.",
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Jan 24, 2017

Choose a reason for hiding this comment

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

please update summary and description

veronicagg
veronicagg approved these changes Jan 25, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

If you're happy with the latest descriptions for the APIs, rest looks good to me.

@veronicagg veronicagg merged commit 0e7ec92 into Azure:master Jan 25, 2017
@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Jan 25, 2017

No modification for Ruby

@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Jan 25, 2017

No modification for Python

@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Jan 25, 2017

No modification for NodeJS

@nking-1 nking-1 mentioned this pull request Jan 27, 2017
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@nking-1 nking-1 nking-1 left review comments

@veronicagg veronicagg veronicagg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@veronicagg veronicagg

Labels

potential-sdk-breaking-change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@naveedaz @AutorestCI @nking-1 @veronicagg @azurecla