Skip to content
New issue

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

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

Already on GitHub? # to your account

[Storage] Support Managepolicy policy on 2018-03-01-preview #4367

Merged
merged 9 commits into from
May 29, 2018

Conversation

blueww
Copy link
Member

@blueww blueww commented May 25, 2018

Description

This PR is generated from swagger in private repo, will update to public repo when the swagger PR for public repo is merged.
But the code should be the same, so please help to start review now.
The feature target to release on 5/30.

The swagger PR:
Private: pass review on private repo: Azure/azure-rest-api-specs-pr#465
Public: just sent to public repo: Azure/azure-rest-api-specs#3137

Rest API Spec: https://microsoft-my.sharepoint.com/:w:/r/personal/yzheng_ntdev_microsoft_com/_layouts/15/Doc.aspx?sourcedoc=%7B3E66A1C5-12C8-4ACB-BCF0-D59B015DE394%7D&file=DLMPreviewDoc.docx&action=default&mobileredirect=true

Breaking:
Per onesdk team requirement in issue Azure/azure-rest-api-specs#3024, change Usage to Usages, since operation ID should not be singleton.

Upgrade SDK version to 8.0.0-preview since the breaking.


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.

@blueww
Copy link
Member Author

blueww commented May 25, 2018

@shahabhijeet

The build failed since a Test case fail.
It seems the failure is not related with my code change, would you please help to fix it and build?

21:05:12 [xUnit.net 00:00:01.8712997] TestFramework.Tests.TestEnvironment.TestEnvironmentTests.DefaultTenantInTestEnvironment [FAIL]
21:05:12 [xUnit.net 00:00:01.8735540] System.AggregateException : One or more errors occurred. (AADSTS70002: Error validating credentials. AADSTS50012: Invalid client secret is provided.
21:05:12 [xUnit.net 00:00:01.8738628] Trace ID: d8de02bd-3b48-4661-be8a-a0d67c6a1d00
21:05:12 [xUnit.net 00:00:01.8739711] Correlation ID: 671433c7-97df-44d2-865c-ed0731794485
21:05:12 [xUnit.net 00:00:01.8740656] Timestamp: 2018-05-25 04:05:11Z)

GitHub user: Azure
Branch: master
Commit: 96d9da589ae6610fc816fca6b25288519974c83a
GitHub user: blueww
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Please generate the code from the Azure rest spec repo's master branch, we cannot accept PRs from code generated from a REST spec from a different repo/branch
  2. Please create a generate.ps1 similar to this and regenerate the code

Copy link
Member Author

@blueww blueww May 28, 2018

Choose a reason for hiding this comment

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

  1. The PR has been re-generated on public swagger master branch, as the swagger is just merged.
    I send the SDK PR for review first from private repo in advance before swagger PR is merged, since would like to have the PR review sooner to avoid risk of get review comments later.

  2. I have already added generate.ps1 before. And now update it as the sample you provide. But the new script is not tested? Please help to review. Is there a way to test it?

@@ -7,7 +7,7 @@
<PackageId>Microsoft.Azure.Management.Storage</PackageId>
<Description>Microsoft Azure Management Storage Library</Description>
<AssemblyName>Microsoft.Azure.Management.Storage</AssemblyName>
<Version>7.2.0-preview</Version>
<Version>8.0.0-preview</Version>
<PackageTags>Microsoft Azure Storage management;Storage;Storage management;</PackageTags>
<PackageReleaseNotes>See https://aka.ms/asdotnetsdkchangelog for release notes.</PackageReleaseNotes>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please confirm that the package release notes are updated

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is there a reason you plan to release a major version, you can release preview breaking changes by bumping the minor version too

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Where's the package release notes? Do you mean the change log? I have updated it.
  2. I update the major version just for the breaking change. Although we still in preview, we use the GA quality bar.

@@ -1170,12 +1148,27 @@ public void StorageAccountUpdateEncryptionTest()
Assert.Equal(true, account.Encryption.Services.File.Enabled);
Assert.NotNull(account.Encryption.Services.File.LastEnabledTime);

// 2. Update storage encryption by enable Blob Encyrption
//// 2. Explicitly disable file encryption service.
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally discourage commented code. It's best to move this code to a different test and mark it as Skipped

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.
Fixed.

@dsgouda
Copy link
Contributor

dsgouda commented May 29, 2018

@blueww Please pull down latest upstream changes and update the PR

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, will merge on CIs passing

@dsgouda dsgouda merged commit 68a092d into Azure:psSdkJson6 May 29, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants