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 NIC Change Create with Default Security Group Parameter #48481

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@marc-sensenich
Contributor

marc-sensenich commented Nov 10, 2018

SUMMARY
  • Update the create_with_security_group variable to be create_with_default_security_group for clarity
  • Sets create_with_default_security_group and security_group to be mutually exclusive
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

azure_rm_network_interface

ANSIBLE VERSION
$ ansible --version
ansible 2.8.0.dev0 (feature/mutually-exclusive-security-group-azure-nic 074e25c8e3) last updated 2018/11/10 20:39:21 (GMT +000)
  config file = None
  configured module search path = [u'/home/marc/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/marc/github/marc-sensenich/ansible/lib/ansible
  executable location = /home/marc/.pyenv/versions/ansible-development/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]
ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2018

Hi @marc-sensenich, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2018

@marc-sensenich, 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

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_networkinterface.py:444:17: E128 continuation line under-indented for visual indent

click here for bot help

@Fred-sun

This comment has been minimized.

Contributor

Fred-sun commented Nov 12, 2018

@marc-sensenich Thanks for your contribution, Could you help share the reason that sets create_with_default_security_group and security_group to be mutually exclusive? what kink of functionality do you want to implement? Thanks!

@bcoca bcoca removed the needs_triage label Nov 13, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented Nov 16, 2018

- output.changed
- output.state.network_security_group.name == 'nsg{{ rpfx }}'
- name: add an existing NSG to an existing NIC (idempotent)

This comment has been minimized.

@yuwzho

yuwzho Nov 16, 2018

Contributor

I think from Line498 to Line 575 are duplicate with

- name: Create NIC using virtual_network resource_group parameter
azure_rm_networkinterface:
resource_group: "{{ resource_group }}"
name: "tn{{ rpfx }}rg"
virtual_network:
name: "tn{{ rpfx }}"
resource_group: "{{ resource_group_secondary }}"
subnet: "tn{{ rpfx }}"
public_ip_name: "tn{{ rpfx }}rg"
public_ip_allocation_method: Static
security_group: "tn{{ rpfx }}"
register: output
- name: Create NIC using virtual_network resource_group parameter (idempotent)
azure_rm_networkinterface:
resource_group: "{{ resource_group }}"
name: "tn{{ rpfx }}rg"
virtual_network:
name: "tn{{ rpfx }}"
resource_group: "{{ resource_group_secondary }}"
subnet: "tn{{ rpfx }}"
public_ip_name: "tn{{ rpfx }}rg"
public_ip_allocation_method: Static
security_group: "tn{{ rpfx }}"
register: output

We can add the disassociate NSG test after "Create NIC using virtual_network resource_group parameter (idempotent)" to avoid redundant resource created during integration test.

This comment has been minimized.

@marc-sensenich

marc-sensenich Nov 26, 2018

Contributor

Agreed, I'll move the test case

This comment has been minimized.

@Fred-sun

Fred-sun Dec 6, 2018

Contributor

@marc-sensenich Thanks for your contribution, do you finished changed?

@Fred-sun

This comment has been minimized.

Contributor

Fred-sun commented Nov 19, 2018

@marc-sensenich Thanks for the contribution, Could you help recheck this PR according by yuwzho's advice? Thanks!

@ansibot ansibot added the stale_ci label Nov 19, 2018

@Fred-sun

This comment has been minimized.

Contributor

Fred-sun commented Nov 26, 2018

@marc-sensenich Thanks for your contribution, Are you still here? Could you update this PR according by yuwzho's comment? when you finished, I will push to merge? Thanks!

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