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

azure_managed_disk enhancement #50317

Merged
merged 5 commits into from Jan 10, 2019

Conversation

Projects
None yet
5 participants
@yuwzho
Copy link
Contributor

yuwzho commented Dec 26, 2018

SUMMARY

collapse source_uri/source_resource_uri args
enhance tests to validate changes with facts module
managed_by was a breaking change; undefined values cannot detach disk and explicitly detach with empty string

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

azure_rm_managed_disk

ADDITIONAL INFORMATION
2.7.0
@ansibot

This comment has been minimized.

fix
@Fred-sun

This comment has been minimized.

Copy link
Contributor

Fred-sun commented Dec 28, 2018

@@ -74,7 +74,8 @@
managed_by:
description:
- Name of an existing virtual machine with which the disk is or will be associated, this VM should be in the same resource group.
- To detach a disk from a vm, keep undefined.
- To detach a disk from a vm, explicitly set to ''.
- If this option is unset, the value will not be changed.

This comment has been minimized.

@yungezz

yungezz Jan 2, 2019

Contributor

@zikalino this is a common problem also. is this behavior acceptable for you?

This comment has been minimized.

@zikalino

zikalino Jan 10, 2019

Contributor

i think it's much better than current behaviour.

currently, if we have playbook with following tasks:

  • create attached disk
  • create vm with that disk
    it works correctly when we run it for the first time
    but when the same playbook is run for the second time, it will detach the disk and then attach it again,
    while proper ansible behaviour should be "no change" as we run the same playbook.

So the only thing that concerns me is that we are changing module behaviour.

Show resolved Hide resolved lib/ansible/modules/cloud/azure/azure_rm_managed_disk.py

@ansibot ansibot removed the needs_triage label Jan 2, 2019

yuwzho added some commits Jan 2, 2019

@yungezz

yungezz approved these changes Jan 7, 2019

@yungezz

This comment has been minimized.

Copy link
Contributor

yungezz commented Jan 10, 2019

shipit

@ansibot ansibot added shipit and removed community_review labels Jan 10, 2019

@@ -51,17 +51,17 @@
- Premium_LRS
create_option:
description:
- "Allowed values: empty, import, copy. C(import) from a VHD file in I(source_uri) and C(copy) from previous managed disk I(source_resource_uri)."
- "Allowed values: empty, import, copy.

This comment has been minimized.

@zikalino

zikalino Jan 10, 2019

Contributor

hmm... I don't think we need additional description in case if we have choices defined. but that's minor.

@zikalino

This comment has been minimized.

Copy link
Contributor

zikalino commented Jan 10, 2019

shipit

@yungezz yungezz merged commit c405fe3 into ansible:devel Jan 10, 2019

1 check passed

Shippable Run 100420 status is SUCCESS.
Details

@yungezz yungezz deleted the VSChina:yuwzho-disk branch Jan 10, 2019

kbreit added a commit to kbreit/ansible that referenced this pull request Jan 11, 2019

knumskull added a commit to knumskull/ansible that referenced this pull request Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment