Fix for showing on GET and persisting Tags on SET (if not given) for Database, Server and Elastic Pool #3206

vaishali33 · 2016-11-17T20:19:58Z

Description

Fix for showing on GET and persisting Tags on SET (if not given) for Database, Server and Elastic Pool.
Also fixed Set-AzureRmSqlServer if SqlAdministratorPassword is not provided.

Fixes #1134 :Fetching Tags from Get-AzureRmSqlDatabase to set Tags values in set-AzureRmSqlDatabase if not provided

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

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.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThrough parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

…ET and SET. Fixed SET server command.

…into dev

azurecla · 2016-11-17T20:20:07Z

Hi @vaishali33, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (vanark). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

azuresdkci · 2016-11-17T20:20:10Z

Can one of the admins verify this patch?

jaredmoo · 2016-11-17T20:53:26Z

src/ResourceManager/Sql/Commands.Sql/Database/Cmdlet/SetAzureSqlDatabase.cs

@@ -113,7 +113,7 @@ protected override IEnumerable<AzureSqlDatabaseModel> ApplyUserInputToModel(IEnu
                Edition = Edition,
                MaxSizeBytes = MaxSizeBytes,
                RequestedServiceObjectiveName = RequestedServiceObjectiveName,
-                Tags = TagsConversionHelper.CreateTagDictionary(Tags, validate: true),
+                Tags = this.MyInvocation.BoundParameters.ContainsKey("Tags") ? TagsConversionHelper.CreateTagDictionary(Tags, validate: true) : model.FirstOrDefault().Tags,


If .FirstOrDefault returns null, .Tags will throw NullRefException. I think that model should always have exactly one element here, if this is correct then use Single instead of FirstOrDefault

Since this line is a little complicated and is repeated several times, I suggest extracting it as a common method

I see that we are using .FirstOrDefault in code just two lines below model.FirstOrDefault().Location. model.FirstOrDefault() should not be null or else we will not reach to this point in code.

For second comment I don't think this its complex statement and its only used at 3 places. Doesn't make sense to extract that to a method in some base class.

If you know that the collection is non-empty, then using FirstOrDefault() is just confusing to the reader - use First() instead.

The exact same code in 3 places is enough justification to extract method. It can be in utility class or base class, your choice. The fact that the code is complex enough to have a bug once (this time) means that it's complex enough to have a bug again, and when that happens the fix should only be done in one place.

Comments addressed.

hovsepm · 2016-11-18T23:34:49Z

@azuresdkci test this please

hovsepm · 2016-11-18T23:36:08Z

@azuresdkci add to whitelist.

markcowl

Also, please respond to jaredmoo review comments and please add a test to cover this scenario - it can be a unit test or a scenario test

markcowl · 2016-11-22T01:23:09Z

src/ResourceManager/Sql/Commands.Sql/Server/Cmdlet/SetAzureSqlServer.cs

@@ -95,12 +95,13 @@ protected override IEnumerable<Model.AzureSqlServerModel> ApplyUserInputToModel(
        {
            // Construct a new entity so we only send the relevant data to the server
            List<Model.AzureSqlServerModel> updateData = new List<Model.AzureSqlServerModel>();
+


remove extra line

markcowl · 2016-11-22T20:02:48Z

@vaishali33 Please respond to review comments, please read and fill out the checklist.

shahabhijeet · 2016-11-28T19:04:45Z

@vaishali33 please address the PR. Code complete is next week.

vaishali33 · 2016-11-28T21:58:08Z

@markcowl and @shahabhijeet I have addressed the comments and added tests. Please review. Thank you!

# Conflicts: # src/ResourceManager/Sql/ChangeLog.md

…into dev # Conflicts: # src/ResourceManager/Sql/ChangeLog.md

jaredmoo · 2016-11-29T20:41:02Z

Thanks for the changes Vaishali, LGTM!

Vaishali Narkhede added 3 commits November 16, 2016 16:34

Fixed Azure Sql Tags for Database, server and elastic pools.

e8b31b1

Fixed Azure Sql Tags for Database, server and elastic pools on both G…

02f9bf0

…ET and SET. Fixed SET server command.

Merge branch 'dev' of https://github.com/vaishali33/azure-powershell …

cbbcd1b

…into dev

azurecla added the cla-not-required label Nov 17, 2016

jamestao added this to the December 2016 milestone Nov 17, 2016

jaredmoo reviewed Nov 17, 2016

View reviewed changes

markcowl requested changes Nov 22, 2016

View reviewed changes

markcowl removed this from the December 2016 milestone Nov 22, 2016

Added tests and addressed comments from code review.

88c5599

Vaishali Narkhede added 3 commits November 29, 2016 12:03

Added tests and addressed comments from code review.

24a5c68

Merge branch 'dev' of https://github.com/Azure/azure-powershell into dev

357498d

# Conflicts: # src/ResourceManager/Sql/ChangeLog.md

Merge branch 'dev' of https://github.com/vaishali33/azure-powershell …

1b8af0b

…into dev # Conflicts: # src/ResourceManager/Sql/ChangeLog.md

shahabhijeet merged commit 3d906ff into Azure:dev Nov 30, 2016

Fix for showing on GET and persisting Tags on SET (if not given) for Database, Server and Elastic Pool #3206

Fix for showing on GET and persisting Tags on SET (if not given) for Database, Server and Elastic Pool #3206

Uh oh!

Conversation

vaishali33 commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azurecla commented Nov 17, 2016

Uh oh!

azuresdkci commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaishali33 Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Nov 18, 2016

Uh oh!

hovsepm commented Nov 18, 2016

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Nov 22, 2016

Uh oh!

shahabhijeet commented Nov 28, 2016

Uh oh!

vaishali33 commented Nov 28, 2016

Uh oh!

jaredmoo commented Nov 29, 2016

Uh oh!

Uh oh!

vaishali33 commented Nov 17, 2016 •

edited

Loading

vaishali33 Nov 22, 2016 •

edited

Loading