Skip to content
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_managed_disk.py --add zones #53788

Open
wants to merge 10 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@Fred-sun
Copy link
Contributor

Fred-sun commented Mar 14, 2019

Implement Availability Zones for managed disk (fixes #51321)

SUMMARY
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_managed_disk.py

ADDITIONAL INFORMATION

Update azure_rm_managed_disk.py
add zones feature
@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 14, 2019

@Fred-sun, just so you are aware we have a dedicated Working Group for azure.
You can find other people interested in this in #ansible-azure on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@yuwzho
Copy link
Contributor

yuwzho left a comment

The PR looks good 👍 Please also add a test.

BTW, to refer the issue, use fixes instead of fixed by

@@ -220,6 +224,9 @@ def __init__(self):
),
managed_by=dict(
type='str'
),
zones=dict(
type='list'

This comment has been minimized.

@yuwzho

yuwzho Mar 14, 2019

Contributor

type='list', elements='str'

This comment has been minimized.

@yuwzho

yuwzho Mar 15, 2019

Contributor

add elements type

This comment has been minimized.

@yuwzho

yuwzho Mar 20, 2019

Contributor

?

Show resolved Hide resolved lib/ansible/modules/cloud/azure/azure_rm_managed_disk.py
@@ -258,6 +266,8 @@ def exec_module(self, **kwargs):
resource_group = self.get_resource_group(self.resource_group)
if not self.location:
self.location = resource_group.location
# convert elements to ints
self.zones = [int(i) for i in self.zones] if self.zones else None

This comment has been minimized.

@yuwzho

yuwzho Mar 14, 2019

Contributor

remove this line and add it to spec. BTW, the SDK accept the zones as [str]

@@ -240,6 +247,7 @@ def __init__(self):
self.os_type = None
self.disk_size_gb = None
self.tags = None
self.tags = None

This comment has been minimized.

@yuwzho

yuwzho Mar 14, 2019

Contributor

redundant

@@ -335,6 +345,7 @@ def generate_managed_disk_property(self):
creation_data = {}
disk_params['location'] = self.location
disk_params['tags'] = self.tags
disk_params['zones'] = self.zones

This comment has been minimized.

@yuwzho

yuwzho Mar 14, 2019

Contributor

Please also update the comments at line 343

This comment has been minimized.

@yuwzho

yuwzho Mar 15, 2019

Contributor

remove the corresponding comments

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 14, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_managed_disk.py:91:1: W293 blank line contains whitespace

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_managed_disk.py:0:0: E309 version_added for new option (zones) should be '2.8'. Currently StrictVersion ('0.0')

click here for bot help

Fred-sun added some commits Mar 14, 2019

@ansibot ansibot removed the ci_verified label Mar 14, 2019

Fred-sun added some commits Mar 14, 2019

@Fred-sun

This comment has been minimized.

Copy link
Contributor Author

Fred-sun commented Mar 14, 2019

@yuwzho Thanks Yuwzho, I have updated it again, please help to check!

@@ -100,6 +104,7 @@
location: eastus
resource_group: myResourceGroup
disk_size_gb: 4
zones: 2

This comment has been minimized.

@yuwzho

yuwzho Mar 14, 2019

Contributor

This sample doesn't match the doc

@yuwzho

This comment has been minimized.

Copy link
Contributor

yuwzho commented Mar 14, 2019

Please also address the other 4 comments. BTW the integration test is under https://github.com/ansible/ansible/blob/devel/test/integration/targets/azure_rm_managed_disk/tasks/main.yml

Fred-sun added some commits Mar 14, 2019

@Fred-sun

This comment has been minimized.

Copy link
Contributor Author

Fred-sun commented Mar 15, 2019

Thanks Yuwei, I submit the new test!

@Fred-sun

This comment has been minimized.

Copy link
Contributor Author

Fred-sun commented Mar 15, 2019

ready_for_review

@yuwzho
Copy link
Contributor

yuwzho left a comment

Please also add the sample

@@ -220,6 +224,9 @@ def __init__(self):
),
managed_by=dict(
type='str'
),
zones=dict(
type='list'

This comment has been minimized.

@yuwzho

yuwzho Mar 15, 2019

Contributor

add elements type

@@ -335,6 +345,7 @@ def generate_managed_disk_property(self):
creation_data = {}
disk_params['location'] = self.location
disk_params['tags'] = self.tags
disk_params['zones'] = self.zones

This comment has been minimized.

@yuwzho

yuwzho Mar 15, 2019

Contributor

remove the corresponding comments

name: "md{{ rpfx }}3"
storage_account_type: "Standard_LRS"
disk_size_gb: 1
zones: 1

This comment has been minimized.

@yuwzho

yuwzho Mar 15, 2019

Contributor

this is not a list

zone:
   - 1
   - 2
@yuwzho

This comment has been minimized.

Copy link
Contributor

yuwzho commented Mar 15, 2019

Please add ready_for_review tag after addressing all comments

@Fred-sun

This comment has been minimized.

Copy link
Contributor Author

Fred-sun commented Mar 20, 2019

fixes #51321

@yuwzho
Copy link
Contributor

yuwzho left a comment

Please modify the zones to zones=dict(type='list', elements='int') in arg_spec according to the comments

@@ -220,6 +224,9 @@ def __init__(self):
),
managed_by=dict(
type='str'
),
zones=dict(
type='list'

This comment has been minimized.

@yuwzho

yuwzho Mar 20, 2019

Contributor

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.