Skip to content

Move ARM cmdlets from Resources to Resources.Rest proj #2125

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

Merged
merged 102 commits into from
May 26, 2016

Conversation

vivsriaus
Copy link
Contributor

This is a massive change list that impacts the core ARM cmdlets like ResourceGroups, deployments, features and providers. We're moving these cmdlets which are currently using the old HYAK client into the Commands.Rest project, using the new autorest SDK client. More updates will follow shortly

@vivsriaus
Copy link
Contributor Author

@azuresdkci test this please

@vivsriaus
Copy link
Contributor Author

@azuresdkci retest this please

…MoveRG

# Conflicts:
#	src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/SdkModels/Deployments/TemplateValidationInfo.cs
#	src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/SdkModels/Errors/PSResourceManagerError.cs
#	src/ResourceManager/Resources/Commands.ResourceManager/Cmdlets/SdkModels/ResourceProvider/PSResourceProviderLocationInfo.cs
#	src/ResourceManager/Resources/Commands.Resources.Test/Commands.Resources.Test.csproj
#	src/ResourceManager/Resources/Commands.Resources.Test/Models.ResourceGroups/ResourceClientTests.cs
#	src/ResourceManager/Resources/Commands.Resources.Test/packages.config
#	src/ResourceManager/Resources/Commands.Resources/Commands.Resources.csproj
#	src/ResourceManager/Resources/Commands.Resources/Models.ResourceGroups/PSResourceProviderLocation.cs
#	src/ResourceManager/Resources/Commands.Resources/Models.ResourceGroups/PSResourceProviderLocationInfo.cs
#	src/ResourceManager/Resources/Commands.Resources/Models.ResourceGroups/ResourceClient.ResourceManager.cs
#	src/ResourceManager/Resources/Commands.Resources/Models.ResourceGroups/ResourceClient.cs
#	src/ResourceManager/Resources/Commands.Resources/Models.ResourceGroups/ResourcesExtensions.cs
#	src/ResourceManager/Tags/Commands.Tags/Commands.Tags.csproj
#	src/ResourceManager/Tags/Commands.Tags/packages.config
…s test

Fix RM test controller and build issues

Merge branch 'dev' of https://github.com/Azure/azure-powershell into MoveRG
@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

files in src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroups have very long lines which makes it hard to view on github, please limit them to 100~120 symbols per line.

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

in src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroups\GetAzureResourceGroupCmdlet.cs, line 79. Instead of catching all the exceptions and silently continuing please write logs at debug stream - aka WriteDebug

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\SdkClient\ResourceManagerSdkClient.cs -> add license header.

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

the naming convention for PS wrappers of model class is PS[nameOfTheModelToWrap]. you have several classes that violates that. Please fix:

  • CreatePSResourceGroupDeploymentParameters.cs
  • ValidatePSResourceGroupDeploymentParameters.cs
  • CreatePSResourceGroupParameters.cs
  • UpdatePSResourceGroupParameters.cs

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Utilities\TemplateUtility.cs - missing license header.

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Utilities\TemplateUtility.cs, line 108. Please add exception details through WriteDebug.

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Utilities\TemplateUtility.cs, line 172. Change Debug.Assert to argument exception

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

due to huge changes please bump up major version in AzureRM.Resources.psd1

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

move strings to resources:

  • src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroupDeployments\NewAzureResourceGroupDeploymentCmdlet.cs line 69, 76
  • src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroups\FindAzureResourceGroupCmdlet.cs line 78
  • src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroups\NewAzureResourceGroupCmdlet.cs line 46
  • \src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\Implementation\ResourceGroups\SetAzureResourceGroupCmdlet.cs line 54
  • src\ResourceManager\Resources\Commands.ResourceManager\Cmdlets\SdkClient\ResourceManagerSdkClient.cs line 212, 213, 301, 595, 624, 784, 861, 865, 874,

<value>Are you sure you want to cancel the active deployment of resource group '{0}'?</value>
</data>
<data name="CancelResourceGroupDeploymentMessage" xml:space="preserve">
<value>Cancelling active resource group deployment ...</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no space after "deployment" and "..." or get line 121 in sync with this line

@hovsepm
Copy link
Contributor

hovsepm commented May 24, 2016

src\ResourceManager\Resources\Commands.Resources.Test\ScenarioTests\ResourcesController.cs, you should introduce an overload for RunPsTest which takes XunitTracingInterceptor as first parameter and then assigns it to helper.TracingInterceptor and should use that method from tests. (e.g. https://github.com/Azure/azure-powershell/blob/dev/src/ResourceManager/Profile/Commands.Profile.Test/ProfileController.cs#L57). This will let you have output logs from PS code execution.

@vivsriaus
Copy link
Contributor Author

@hovsepm I've addressed all the feedback, except for the last one: Adding the XunitTracingInterceptor to our test controller. I'm not comfortable making this change yet, as I don't know what the impact of this change will be on other RP projects that depend on this. If the above changes are merged, I will then be comfortable working on this tomorrow. Let me know if that sounds good.

@vivsriaus
Copy link
Contributor Author

@vivsriaus
Copy link
Contributor Author

@vivsriaus
Copy link
Contributor Author

@vivsriaus
Copy link
Contributor Author

@hovsepm
Copy link
Contributor

hovsepm commented May 25, 2016

@vivsriaus "This branch has conflicts that must be resolved" please pull from upstream and resolve conflicts

…MoveRG

Conflicts:
	src/ResourceManager/Resources/Commands.Resources.Test/Commands.Resources.Test.csproj
@vivsriaus
Copy link
Contributor Author

vivsriaus commented May 25, 2016

@vivsriaus
Copy link
Contributor Author

@hovsepm things looking good on this pr?

@hovsepm
Copy link
Contributor

hovsepm commented May 25, 2016

@vivsriaus reviewing.

@hovsepm
Copy link
Contributor

hovsepm commented May 25, 2016

@hovsepm hovsepm merged commit 18be449 into Azure:dev May 26, 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.

10 participants