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

adding support for backing up and restoring KeyVault secrets #3855

Closed
wants to merge 0 commits into from

Conversation

dragav
Copy link
Contributor

@dragav dragav commented Apr 28, 2017

adding support for backing up and restoring of KeyVault secrets

revising KeyVault backup/restore cmdlets:
adding support for pipelining
allowing overwriting of dest file
using session data store for file operations

@azuresdkci
Copy link

Can one of the admins verify this patch?

@msftclas
Copy link

@dragav,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@cormacpayne
Copy link
Member

Old PR: #3787

@cormacpayne
Copy link
Member

@azuresdkci add to whitelist

@@ -253,6 +255,10 @@
<Project>{3819d8a7-c62c-4c47-8ddd-0332d9ce1252}</Project>
<Name>Commands.ResourceManager.Common</Name>
</ProjectReference>
<ProjectReference Include="..\..\Profile\Commands.Profile\Commands.Profile.csproj">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this reference?

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the reference to Commands.Profile, this is discouraged in cmdlet projects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I added this reference so as not to duplicate the FileOverwrite messages. That couldn't be avoided, though. Reverted.

@@ -10,8 +10,8 @@

namespace Microsoft.Azure.Commands.KeyVault.Properties {
using System;



Copy link
Contributor

Choose a reason for hiding this comment

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

for resource.designer.cs and resource.resx there are additional space added that are unnecessary. Can you please revert them?

public string BackupSecret( string vaultName, string secretName, string outputBlobPath )
{
if ( string.IsNullOrEmpty( vaultName ) )
throw new ArgumentNullException( "vaultName" );
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. use nameof(vaultName) instead.

ParameterSetName = BySecretObjectParameterSet,
HelpMessage = "Output file. The output file to store the backed up secret blob in. If not present, a default filename is chosen." )]
[ValidateNotNullOrEmpty]
public string OutputFile { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to parameters for the parameter sets as they have the same properties. Just remove ParameterSetName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks; addressed in a subsequent commit, should show up shortly.

pomortaz
pomortaz previously approved these changes Apr 28, 2017
Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

One comment, also please add a description of the changes in changelog.md

@@ -253,6 +255,10 @@
<Project>{3819d8a7-c62c-4c47-8ddd-0332d9ce1252}</Project>
<Name>Commands.ResourceManager.Common</Name>
</ProjectReference>
<ProjectReference Include="..\..\Profile\Commands.Profile\Commands.Profile.csproj">
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the reference to Commands.Profile, this is discouraged in cmdlet projects

@markcowl
Copy link
Member

@dragav There are merge conflicts, please update

1 similar comment
@markcowl
Copy link
Member

@dragav There are merge conflicts, please update

@dragav
Copy link
Contributor Author

dragav commented May 2, 2017

I picked up a bunch of commits following a faulty merge. Resubmitting a clean PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants