Update azure_rm_storageaccount.py

[paultaiton]

SUMMARY

Fix passing minimum_tls_version, allow_blob_public_access, and https_only on new storage account creation.

Fixes #457
Fixes #325

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

azure_rm_storageaccount

ADDITIONAL INFORMATION

This is a bugfix in the creation of new storage account, and update of tests to catch this bug in future regressions.

[paultaiton]

@Fred-sun,
I believe you wrote the original code that I'm modifying in this PR. Your review would be appreciated.

@arsenicks,
This should correct the problem you have in the short term, until I can complete the more comprehensive PR.

[arsenicks]

@paultaiton Ohhh thanks a lot for this!!

[Fred-sun]

@paultaiton I have checked the parameter in azure-cli, https-only default value is true. Please update follow the comment and update the doc---line 90. Thank you very much!

azure/plugins/modules/azure_rm_storageaccount.py

Line 90 in bb56aec

- Default value is C(False).

link: https://docs.microsoft.com/en-us/cli/azure/storage/account?view=azure-cli-latest#az_storage_account_create

[paultaiton]

If we are setting https_only parameter to be True by default (which makes sense to match storageaccount API updates 2019-06-01,) then the line 826

         if not bool(self.https_only): 
             self.https_only = False 

is no longer necessary, since the property can only ever be True or False, and never None.

I've modified the code to match this convention instead.

[Fred-sun]

If we are setting https_only parameter to be True by default (which makes sense to match storageaccount API updates 2019-06-01,) then the line 826

         if not bool(self.https_only): 
             self.https_only = False 

is no longer necessary, since the property can only ever be True or False, and never None.

I've modified the code to match this convention instead.

Yes! agree with your comments!

[Fred-sun]

Please

[paultaiton]

@Fred-sun
I added a couple more changes since I noticed that minimum_tls_version had the same bug, and to make blob_public_access more explicit and obvious with the defaults.

[Fred-sun]

Please!

[Fred-sun]

not need! The default value for this property is null

[Fred-sun]

@paultaiton Please update the follow lines, If set minimum_tls_version default value!, It will update the minimum_tls_version value when update account blob public access. advice change to '- output.storageaccounts[0].minimum_tls_version == "TLS1_0" '

azure/tests/integration/targets/azure_rm_storageaccount/tasks/main.yml

Line 190 in 553cbd2

- output.storageaccounts[0].minimum_tls_version == "TLS1_2"

[arsenicks]

@Fred-sun
I added a couple more changes since I noticed that minimum_tls_version had the same bug, and to make blob_public_access more explicit and obvious with the defaults.

I was aware of it and can confirm it's not working as expected in 1.4 but didn't want to bother you with this in the short term as you were working on the whole thing :P Anyway thanks for fixing it, let me know if you need any kind of test. We have a policy in place that was blocking the creation because of the default ACL not being Deny, my first debug lead me to think this is not the same problem but if you can take a second to lookup that part of the code and see if it's similar that would be great.

I'll create another issue for the ACL when I have more information, it's not clear what cause the problem right now.

One question for you guy's, not putting any pressure, just want to know how released are managed with the collection. Do you ship a new collection version on a specific "schedule" or just as needed ? If so is there a planned released in the coming weeks ?

Thanks a lot to you both!

[Fred-sun]

@arsenicks There will probably be an edition of Collections in a month or two.

[Fred-sun]

Please!

[paultaiton]

Hey @Fred-sun ,
The test that failed is correct, it uncovered a bug I introduced.
Luckily I finally figured out how to get these tests running locally, so I'm updating both the integration tests and my code to account for omitting parameters while updating existing accounts. Please hold off on reviewing anything until I comment again, as I've corrected many of the bugs that haven't been pushed up yet.

[paultaiton]

@Fred-sun

OK, I have reorganized and updated the tests to include more checks that should catch similar bugs in the future. It now creates and updates two storage accounts using implicit defaults and explicit settings for creating new accounts and updating existing accounts.

I added to documentation to explain what happens during parameter omission, but left it to use the API (also matches CLI) behavior.

I've run the integration tests and everything was successful, so I think it's ready for review for everything within the scope of this immediate fix. I did uncover a problem with the returned values for network_acls and blob_cors (integration test lines 227-229 that are commented out,) but it's outside the scope of the code I've touched, and I'm not familiar enough with its operation to fix it within this PR. I'll try and get to it later if I can.

Appreciate your help Fred.

[l3ender]

@paultaiton Would you mind sharing detail on how you were able to get tests running locally? I've been trying to do the same but haven't had luck. Any detail you can share on running specific tests and the test suite would be appreciated!

[paultaiton]

@paultaiton Would you mind sharing detail on how you were able to get tests running locally? I've been trying to do the same but haven't had luck. Any detail you can share on running specific tests and the test suite would be appreciated!

Yes, absolutely. I was already planning on making a Documentation PR that describes exactly how to set up an environment for linting and integration tests.

[paultaiton]

Thanks again for your help @haiyuazhang and @Fred-sun . Your support is appreciated.

@arsenicks please let me know if you see any trouble after switching to this new module update.

[l3ender]

@paultaiton Is there any detail you can share in the meantime on how to set up an environment for linting and integration tests? Being able to lint/test on my local environment would help tremendously. Once getting it set up, I'm happy to create and/or contribute to a documentation PR.

[paultaiton]

@l3ender

I use pycodestyle in vscode with --max-line-length=160 and
--ignore=E###,W###

As command line arguments (settings in python extension.) I forget the E number, but the one for module imports not at top of file, and the warn for operators at start of new line. The latter is acceptable according to PEP8, so I consider it a fault of the pycodestyle lint.

The ansible-test bit I'll have to get you when I'm back home, as there is a cloud config file you need to define that I can't recall from memory.