Skip to content

Conversation

@dragonfly91
Copy link

Hi,

This PR comprises of a draft version of the new module - RecoveryServices.Backup. We wanted to get an early feedback on various aspects of the code / design etc. Please review the PR but do NOT merge the PR - since this changeset contains only a few cmdlets. Look forward to your feedback.

Thanks,
Anudeep.

@hovsepm hovsepm changed the title Recovery Services Backup Initial PR [Do NOT merge][Review] Recovery Services Backup Initial PR Mar 31, 2016
@hovsepm hovsepm changed the title [Do NOT merge][Review] Recovery Services Backup Initial PR [Do NOT merge] Recovery Services Backup Initial PR Mar 31, 2016
@dragonfly91 dragonfly91 changed the title [Do NOT merge] Recovery Services Backup Initial PR Recovery Services Backup Initial PR Apr 6, 2016
@hovsepm
Copy link
Contributor

hovsepm commented Apr 7, 2016

@dragonfly91 What is relationship between changes in AzureBackup and RecoveryServices and adding RecoveryServices.Backup module to the repository? could they go in independent of each other?

@hovsepm
Copy link
Contributor

hovsepm commented Apr 7, 2016

@dragonfly91 please change the overall folder structure. Take a look to src\ResourceManager\RecoveryServices project -> product and Test should be in a different folders.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 7, 2016

Your cmdlets should not define 2 output types for lists and for single types.

OutputType(typeof(List<AzureRmRecoveryServicesContainerBase>), typeof(AzureRmRecoveryServicesContainerBase))

do not define List, instead when you do write object specify that it can be an enumerable collection. e.g.

WriteObject(containerModels, enumerateCollection: true);

@hovsepm
Copy link
Contributor

hovsepm commented Apr 7, 2016

@dragonfly91 you should NOT use things like

Thread.Sleep(15 * 1000); 

things (e.g. StopAzureRmRecoveryServicesJob.cs file).
Use TestMockSupport.Delay method from Commands.Common project.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 8, 2016

Move Commands.RecoveryServices.Backup.Helpers and other projects to separate folders under RecoveryServices.Backup folder. Do not put projects in nested subfolders.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 8, 2016

Why do you have so many library packages that are only referenced in one module?

@hovsepm
Copy link
Contributor

hovsepm commented Apr 8, 2016

you should not reference anything from GAC (e.g. Commands.RecoveryServices.Backup.Cmdlets.csproj)

    <Reference Include="System.Management.Automation, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <SpecificVersion>False</SpecificVersion>
      <HintPath>C:\Windows\Microsoft.NET\assembly\GAC_MSIL\System.Management.Automation\v4.0_3.0.0.0__31bf3856ad364e35\System.Management.Automation.dll</HintPath>
    </Reference>

remove the hint path.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 15, 2016

@dragonfly91 since you guys added your module to AzureRM and also added psd1 file for RecoveryServices.Backup you will need to regenerate wxi file and include it to this PR as well.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 27, 2016

@hovsepm
Copy link
Contributor

hovsepm commented Apr 27, 2016

@hovsepm
Copy link
Contributor

hovsepm commented Apr 27, 2016

@hovsepm
Copy link
Contributor

hovsepm commented Apr 27, 2016

@hovsepm
Copy link
Contributor

hovsepm commented Apr 28, 2016

@hovsepm
Copy link
Contributor

hovsepm commented Apr 28, 2016

<Optimize>true</Optimize>
<OutputPath>bin\Release\</OutputPath>
<DefineConstants>TRACE</DefineConstants>
<OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.RecoveryServices.Backup\</OutputPath>
Copy link
Contributor

@hovsepm hovsepm Apr 28, 2016

Choose a reason for hiding this comment

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

revert this line

@hovsepm
Copy link
Contributor

hovsepm commented Apr 28, 2016

@markcowl
Copy link
Member

@markcowl markcowl merged commit a0c503e into Azure:dev Apr 28, 2016
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.