Skip to content

Adding POST APIs used to Upload customer TDE certificates in CMS.#4461

Merged
dsgouda merged 3 commits intoAzure:psSdkJson6from
nivimsft:uploadCustomerTdeCertificate
Jun 21, 2018
Merged

Adding POST APIs used to Upload customer TDE certificates in CMS.#4461
dsgouda merged 3 commits intoAzure:psSdkJson6from
nivimsft:uploadCustomerTdeCertificate

Conversation

@nivimsft
Copy link
Copy Markdown
Contributor

@nivimsft nivimsft commented Jun 18, 2018

Commit contains appropriate tests

Description

Adding POST APIs used to Upload customer TDE certificates in CMS code and tests
PR for spec changes Azure/azure-rest-api-specs#3248

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.

dsgouda
dsgouda reviewed Jun 18, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Will approve once REST spec is merged and sdk regenerated.

jaredmoo
jaredmoo reviewed Jun 18, 2018
View reviewed changes
src/SDKs/SqlManagement/Management.Sql/Microsoft.Azure.Management.Sql.csproj Outdated
Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Jun 18, 2018

Choose a reason for hiding this comment

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

I published 1.17 today (on request from vulnerability assessment team), so now you need to bump this up to 1.18 and wipe out the old release notes (i.e. just line 15)

Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Jun 18, 2018

Choose a reason for hiding this comment

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

Bump to 1.18 both in this file and in AssemblyInfo.cs

Copy link
Copy Markdown
Contributor Author

@nivimsft nivimsft Jun 19, 2018

Choose a reason for hiding this comment

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

Sure will do

jaredmoo
jaredmoo suggested changes Jun 18, 2018
View reviewed changes
src/SDKs/_metadata/sql_resource-manager.txt Outdated
Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Jun 18, 2018

Choose a reason for hiding this comment

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

You will need to generate from Azure/master before this PR can be merged

Copy link
Copy Markdown
Contributor Author

@nivimsft nivimsft Jun 19, 2018

Choose a reason for hiding this comment

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

Sounds good! Waiting for PR to be merged

src/SDKs/SqlManagement/Sql.Tests/UploadTdeCertificateScenarioTests.cs Outdated
Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Jun 18, 2018

Choose a reason for hiding this comment

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

Tests MUST be fully automated. This means that they must create all prerequisite resources during test setup. If this is not possible, the test must be skipped (see geo restore scenario test for example)

Copy link
Copy Markdown
Contributor Author

@nivimsft nivimsft Jun 19, 2018

Choose a reason for hiding this comment

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

Okay, automatic provisioning of Managed instances is not currently possible. Managed instances team is working on this. I will go ahead and skip these tests for now

nivimsft added 2 commits June 19, 2018 17:15
@nivimsft
Adding POST APIs used to Upload customer TDE certificates in CMS.
8dd1346 
Commit contains appropriate tests
@nivimsft
Addressing comments from Jared:
4784ca1 
- Updating version to 1.18 in .csproj and assemblyInfo
- regenerate client from Azure/Master,
- skipping tests with long setup time
dsgouda
dsgouda approved these changes Jun 21, 2018
View reviewed changes
Copy link
Copy Markdown
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

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Jun 21, 2018

Will merge when CIs pass

jaredmoo
jaredmoo reviewed Jun 21, 2018
View reviewed changes
src/SDKs/SqlManagement/Management.Sql/Microsoft.Azure.Management.Sql.csproj Outdated
New features:
- Added support for using sever level rule in vulnerability assessment baseline operations.
- Added support for using sever level Threat Detection.
- Enable customers to upload their TDE certificate to CMS
Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Jun 21, 2018

Choose a reason for hiding this comment

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

Customers do not know what CMS is. This release note should be more like, "Added support for uploading TDE certificate into servers and managed instances."

Copy link
Copy Markdown
Contributor Author

@nivimsft nivimsft Jun 21, 2018

Choose a reason for hiding this comment

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

You are right, will address

jaredmoo
jaredmoo approved these changes Jun 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo left a comment

Choose a reason for hiding this comment

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

Looks good, release note needs a little edit.

@jaredmoo
Copy link
Copy Markdown
Contributor

jaredmoo commented Jun 21, 2018

Thanks, now it's perfect!

@nivimsft
Copy link
Copy Markdown
Contributor Author

nivimsft commented Jun 21, 2018

I like that, thanks for your help in making it so! :)

@dsgouda dsgouda merged commit d1a925f into Azure:psSdkJson6 Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jaredmoo jaredmoo jaredmoo approved these changes

@dsgouda dsgouda dsgouda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nivimsft @dsgouda @jaredmoo