Skip to content

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

Merged
balajikris merged 2 commits intoAzure:currentfrom
ScottHolden:current
Aug 9, 2017
Merged

Microsoft.Sql - Added data sync to package-2015-05-preview#1510
balajikris merged 2 commits intoAzure:currentfrom
ScottHolden:current

Conversation

@ScottHolden
Copy link
Copy Markdown
Contributor

@ScottHolden ScottHolden commented Aug 7, 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

@msftclas
Copy link
Copy Markdown

msftclas commented Aug 7, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@azuresdkciprbot
Copy link
Copy Markdown

azuresdkciprbot commented Aug 7, 2017

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

balajikris commented Aug 7, 2017

@ScottHolden - I assume those specs were reviewed and merged earlier? this PR is about adding those to readme so that autorest generates SDKs for them?

@balajikris
Copy link
Copy Markdown
Contributor

balajikris commented Aug 7, 2017

Hey @jaredmoo - I think you added advisors.json. I wonder if it was missed while the transition to .md happened and if so, does this change look good to you? It looks good to me but I'm trying to understand the root cause here.

@jaredmoo
Copy link
Copy Markdown
Contributor

jaredmoo commented Aug 7, 2017 •
edited
Loading

Thanks for the contribution Scott!

All code in azure-sdk-for-net must have scenario tests, so I have been holding off on adding Swagger files to readme.md until we have a PR to add scenario tests to azure-sdk-for-net. Otherwise we get into a tricky situation where one feature might have tests implemented and ready to ship in azure-sdk-for-net (and other clients) but the release could get blocked by another unrelated feature not having scenario tests yet.

Do you have a particular need for data sync and advisor SDK support in an app you are working on?

@ScottHolden
Copy link
Copy Markdown
Contributor Author

ScottHolden commented Aug 7, 2017

Hey guys,
Sorry for the lack of info in the pull request.

Basically need to use data sync via the .Net Sql Management Client, I just included 'advisors' while I was there. Happy to remove it and trim it back down to just data sync :)

We have been using the data sync client via a fork with these changes for a little while, wanted to pull them back in so we can jump back onto the official release train :)

I'm half way through writing data sync scenario tests for the azure-sdk-for-net Management.Sql, I'll put in a pull request over there when I'm done and link it back to this pull request.

Let me know if you need any more info :)

@jaredmoo
Copy link
Copy Markdown
Contributor

jaredmoo commented Aug 7, 2017

That sounds wonderful! Yes please remove advisors for now.

In case it is helpful, there are scenario tests that were written for the old non-swagger-based sdk at https://github.com/Azure/azure-sdk-for-net/blob/master/src/ResourceManagement/Sql/Sql.Tests/ScenarioTests/Sql2.DataSyncScenarioTest.cs . The test setup code will be very different but it should be possible to adapt the overall scenario and some of the logic there.

Please let me know if you have any questions or issues with writing the SDK test.

@jaredmoo
Copy link
Copy Markdown
Contributor

jaredmoo commented Aug 7, 2017

Looks good, I will approve this when I approve the corresponding azure-sdk-for-net change. :)

@balajikris
Copy link
Copy Markdown
Contributor

balajikris commented Aug 7, 2017

Rock on guys! Looks like I have nothing much to do here, other than merge when @jaredmoo signs off after looking at the SDK change :-) Thanks both of you!

@azuresdkciprbot
Copy link
Copy Markdown

azuresdkciprbot commented Aug 7, 2017

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

@ScottHolden ScottHolden changed the title Microsoft.Sql - Added advisors and data sync to package-2015-05-preview Microsoft.Sql - Added data sync to package-2015-05-preview Aug 8, 2017
@ScottHolden ScottHolden mentioned this pull request Aug 8, 2017
Merged
8 tasks
@ScottHolden
Copy link
Copy Markdown
Contributor Author

ScottHolden commented Aug 8, 2017

Corresponding azure-sdk-for-net pull request can be found at:
Azure/azure-sdk-for-net#3558

@msftclas
Copy link
Copy Markdown

msftclas commented Aug 8, 2017

@ScottHolden, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

@jaredmoo
Copy link
Copy Markdown
Contributor

jaredmoo commented Aug 9, 2017

I approve Azure/azure-sdk-for-net#3558 so I also approve this.

@balajikris balajikris merged commit 342a4d4 into Azure:current Aug 9, 2017
@balajikris
Copy link
Copy Markdown
Contributor

balajikris commented Aug 9, 2017

Thanks @ScottHolden and @jaredmoo

@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Aug 9, 2017

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

@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Aug 9, 2017

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

@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Aug 9, 2017

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

anuchandy pushed a commit that referenced this pull request Aug 14, 2017
@gmainar @anuchandy
New API Version 2017-08-01: Adding Sku property into publicIP and loa…
6d76e5f 
…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)
@gmainar gmainar mentioned this pull request Aug 14, 2017
Merged
10 tasks
anuchandy pushed a commit that referenced this pull request Aug 15, 2017
@gmainar @anuchandy
New API Version 2017-08-01: Adding Sku property into publicIP and loa…
2264e7b 
…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)
mccleanp pushed a commit that referenced this pull request Mar 23, 2022
@sanjaiganesh @ankoons
Changing ImmutableResourceId to read/write instead of readonly for ex…
8d8bd56 
…periment's sake. (#1510)

Co-authored-by: Andrew Koons <ankoons@microsoft.com>
Copilot AI mentioned this pull request Sep 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@balajikris balajikris balajikris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@balajikris balajikris

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ScottHolden @msftclas @azuresdkciprbot @balajikris @jaredmoo @AutorestCI