Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regenerating SDK based on swagger changes on List/Cancel operation for Azure database and Elastic pool #4106

Merged
merged 8 commits into from
Mar 16, 2018

Conversation

payiAzure
Copy link
Contributor

@payiAzure payiAzure commented Mar 5, 2018

… list database/pool operation

Description

This pull request is to regenerate SDK based on the swagger changes on List/Cancel Operation for Azure database and elastic pool.

The related swagger pull request that has been merged in the azure-rest-api-specs are:
Azure/azure-rest-api-specs#2520

Azure/azure-rest-api-specs#2592 (only update the sql-resourcemanager readme)

Azure/azure-rest-api-specs#2640 (refactor sql readme. List/Cancel operation is included in the refactored sql readme version)

[Do no merge] notice in the tile: we're doing the ARM manifest update on production currently for new version API. Will remove it from title and do the merge after the ARM manifest update is done.

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

3 similar comments
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

  • Code needs to be generated from the swagger spec checked into master
  • Package Version needs to be updated

GitHub user: kosta-bizetic
Branch: master
Commit: fb9e1b2a7561e7bd7d697011bd063964f2521861
GitHub user: payiAzure
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pull down latest changes and run generate.cmd again, this should log the commit id here

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, looks like you are generating code from your local branch, please generate the code from the checked in swagger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda Sync with the checked-in swagger and regenerate the sdk from master in the second commit. Please take a look

@shahabhijeet
Copy link
Member

@azuresdkci test this please

@dsgouda
Copy link
Contributor

dsgouda commented Mar 14, 2018

Restarting CI

@dsgouda dsgouda closed this Mar 14, 2018
@dsgouda dsgouda reopened this Mar 14, 2018
@dsgouda
Copy link
Contributor

dsgouda commented Mar 14, 2018

@azuresdkci rebuild

@dsgouda
Copy link
Contributor

dsgouda commented Mar 14, 2018

@azuresdkci retest

@jaredmoo
Copy link
Contributor

jaredmoo commented Mar 15, 2018

Please do the last 2 items from the checklist:

  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

:)

@jaredmoo
Copy link
Contributor

csproj & AssemblyInfo.cs should have minor version bump (e.g. 1.12.0.0 -> 1.13.0.0) and please also edit release notes in csproj (remove old release notes describing 1.12 and add new notes describing 1.13)

…-specs. Bump the api version in Assembly file and csproj
@payiAzure
Copy link
Contributor Author

@jaredmoo Update the pull request, address comments on:

  1. The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  2. The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

Dtu = 250,
Tags = tags
});
Thread.Sleep(TimeSpan.FromSeconds(5));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only sleep when in record mode. Take a look at other tests e.g. Restore tests

                    // No need to sleep if we are playing back the recording.
                    if (HttpMockServer.Mode == HttpRecorderMode.Record)
                    {
                        Thread.Sleep(TimeSpan.FromSeconds(5));
                    }

Copy link
Contributor

Choose a reason for hiding this comment

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

btw how reliable is this test? Does it generally pass every time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, when I run the test for multiple time, it passed every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaredmoo update the test in the new commit of the PR: only sleep when in Record mode

@@ -1,11 +1,13 @@
2018-01-31 10:51:15 UTC

Copy link
Contributor

Choose a reason for hiding this comment

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

Please pull down the latest changes from upstream and run the generate.cmd command again, this should log the commit id 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.

@dsgouda . I did, but the interesting thing is no commit id shown in the file. I tried several times.
my forked azurePayi/azure-sdk-for-net repo is updated with the official azure/azure-sdk-for-net repo, and I run the generate.cmd against official azure/azure-rest-spi-specs
since my swagger changes already were merged to the official azure-rest-spi-specs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda Thanks for your help! Update the PR which has commit id in the metadata file.

@payiAzure payiAzure changed the title [Do NOT MERGE] Regenerating SDK based on swagger changes on List/Cancel operation for Azure database and Elastic pool Regenerating SDK based on swagger changes on List/Cancel operation for Azure database and Elastic pool Mar 15, 2018
@payiAzure
Copy link
Contributor Author

@dsgouda Hi Deepak, can we get this PR review complete asap and merge? The sdk-for-phyton publish has dependency on this

@dsgouda
Copy link
Contributor

dsgouda commented Mar 15, 2018

@payiAzure Please fix the build issues here

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM subject to CI passing

@dsgouda
Copy link
Contributor

dsgouda commented Mar 15, 2018

@azuresdkci ReTest this

@dsgouda
Copy link
Contributor

dsgouda commented Mar 15, 2018

@azuresdkci retest this please

@dsgouda dsgouda merged commit 360a9f5 into Azure:psSdkJson6 Mar 16, 2018
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