-
Notifications
You must be signed in to change notification settings - Fork 330
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
Update azure_rm_storageaccount.py #458
Update azure_rm_storageaccount.py #458
Conversation
@Fred-sun, @arsenicks, |
@paultaiton Ohhh thanks a lot for this!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
- Default value is C(False). |
link: https://docs.microsoft.com/en-us/cli/azure/storage/account?view=azure-cli-latest#az_storage_account_create
Changed https_only parameter to be True by default, and removed the conditional to convert from None type.
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
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
…iton/azure into paultaiton_20210315_storage-https
@Fred-sun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please!
type: bool | ||
default: True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not need! The default value for this property is null
@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" '
|
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! |
@arsenicks There will probably be an edition of Collections in a month or two. |
…iton/azure into paultaiton_20210315_storage-https
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please!
Hey @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. |
@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. |
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. |
@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. |
I use pycodestyle in vscode with --max-line-length=160 and 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. |
SUMMARY
Fix passing minimum_tls_version, allow_blob_public_access, and https_only on new storage account creation.
Fixes #457
Fixes #325
ISSUE TYPE
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.