Fixes some NIC problems: create without nsg, support loadbalancer #38643
Conversation
ansibot
added
azure
cloud
core_review
|
The test The test The test |
ansibot
added
ci_verified
jborean93
removed
the
needs_triage
ansibot
removed
the
ci_verified
|
The test The test |
|
The test |
ansibot
added
core_review
|
Try the PR by install devel branch |
|
How long before we can expect this PR to be merged please? |
yuwzho
changed the title
|
@yuwzho this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibot
removed
core_review
ansibot
added
core_review
|
What is the ETA of the merge please? |
| @@ -52,25 +52,22 @@ | |||
| - Valid azure location. Defaults to location of the resource group. | |||
| default: resource_group location | |||
| required: false | |||
| virtual_network_resource_group: | |||
There was a problem hiding this comment.
will it cause existing user using this parameter break by delete? how about keep as alias?
There was a problem hiding this comment.
this would have been new added in 2.6, current user is using devel version, I think we can have this 'breaking change' in devel version
There was a problem hiding this comment.
is it necessary to introduce the break? I would suggest to keep virtual_network_resource_group. If remove this argument, user will not able to create a NIC with vnet in another resource group.
There was a problem hiding this comment.
This is an agreement we reached in a sync up meeting. Refer resource in other resource group support 3 methods:
- accept resource name, and the resource is in the same resource group
- accept a resource id
- accept a sub dict, which contains resource group and name
Here I remove this option to support the 3 methods. Since this option's version_added is 2.6. That means no user is using this option because this hasn't been released.
There was a problem hiding this comment.
The aliases section means both virtual_network or virtual_network_name can be used when specifying this option. The name change is to bring it more in line with other modules, plus now it can take a dict with more than just the name
| primary: | ||
| description: | ||
| - Whether the ip configuration is the primary one in the list. | ||
| type: bool | ||
| default: 'no' | ||
| version_added: 2.5 | ||
| security_group_name: | ||
| create_with_security_group: |
There was a problem hiding this comment.
about this paramter, if no security_group specified, then a deault one should be created. it seems redundant to have a parameter to control the behavior
There was a problem hiding this comment.
Sure, but we cannot break current behavior: no nsg specified, create a default one
There was a problem hiding this comment.
does that mean there's scenario to create a nic without nsg? interesting
There was a problem hiding this comment.
yes, support user create a NIC without NSG. default behavior is create a default NSG and attach to NIC
| @@ -349,6 +374,7 @@ def nic_to_dict(nic): | |||
| private_ip_allocation_method=dict(type='str', choices=['Dynamic', 'Static'], default='Dynamic'), | |||
| public_ip_address_name=dict(type='str', aliases=['public_ip_address', 'public_ip_name']), | |||
| public_ip_allocation_method=dict(type='str', choices=['Dynamic', 'Static'], default='Dynamic'), | |||
| load_balancer_backend_address_pools=dict(type='list'), | |||
There was a problem hiding this comment.
just curious, isn't this a config in load balancer, why add in azure_network_interface module?
There was a problem hiding this comment.
in loadbalancer SDK, it simply ignore this param. And this API is exposed in NIC.
There was a problem hiding this comment.
I suppose that before the receiving NIC has been created it doesn't make sense to add it to a load balancer. Reduces risk of faulty config atleast.
|
ready_for_review |
1 similar comment
|
ready_for_review |
ansibot
added
affects_2.6
| @@ -52,25 +52,22 @@ | |||
| - Valid azure location. Defaults to location of the resource group. | |||
| default: resource_group location | |||
| required: false | |||
| virtual_network_resource_group: | |||
There was a problem hiding this comment.
is it necessary to introduce the break? I would suggest to keep virtual_network_resource_group. If remove this argument, user will not able to create a NIC with vnet in another resource group.
| security_group: testnic001 | ||
| security_group: | ||
| name: testnic002 | ||
| resource_group: "{{ resource_group_secondary }}" |
| primary: | ||
| description: | ||
| - Whether the ip configuration is the primary one in the list. | ||
| type: bool | ||
| default: 'no' | ||
| version_added: 2.5 | ||
| security_group_name: | ||
| create_with_security_group: |
There was a problem hiding this comment.
does that mean there's scenario to create a nic without nsg? interesting
ansibot
removed
stale_ci
| @@ -52,25 +52,22 @@ | |||
| - Valid azure location. Defaults to location of the resource group. | |||
| default: resource_group location | |||
| required: false | |||
| virtual_network_resource_group: | |||
There was a problem hiding this comment.
The aliases section means both virtual_network or virtual_network_name can be used when specifying this option. The name change is to bring it more in line with other modules, plus now it can take a dict with more than just the name
|
Many thanks for these features! |
…sible#38643) * add loadbalancer * dict check nullable * add default vallue when get list * create backend addr pool * fix the set * fix to dict * fix ideponement * use param security group name when create * nic can has no nsg * add test * fix * fix * fix * add document * add configuration * fix * fix * remove all resources * fix * fix test * add version added * fix lint * fix lint * Fixes some NIC bugs (ansible#39213) * add loadbalancer * dict check nullable * add default vallue when get list * create backend addr pool * fix the set * fix to dict * fix ideponement * use param security group name when create * nic can has no nsg * add test * fix * fix * fix * fix idemponet * add document * fix test * add configuration * fix * fix * remove all resources * fix * fix test * add version added * fix lint * fix lint * fix lint * remove new feature and only submit bugfix * remove useless test * fix * fix indent * Update azure_rm_networkinterface.py * fix comment * support 3 types to specific name and resource group * avoid test racing * fix test * add sample * add resource id test
…sible#38643) * add loadbalancer * dict check nullable * add default vallue when get list * create backend addr pool * fix the set * fix to dict * fix ideponement * use param security group name when create * nic can has no nsg * add test * fix * fix * fix * add document * add configuration * fix * fix * remove all resources * fix * fix test * add version added * fix lint * fix lint * Fixes some NIC bugs (ansible#39213) * add loadbalancer * dict check nullable * add default vallue when get list * create backend addr pool * fix the set * fix to dict * fix ideponement * use param security group name when create * nic can has no nsg * add test * fix * fix * fix * fix idemponet * add document * fix test * add configuration * fix * fix * remove all resources * fix * fix test * add version added * fix lint * fix lint * fix lint * remove new feature and only submit bugfix * remove useless test * fix * fix indent * Update azure_rm_networkinterface.py * fix comment * support 3 types to specific name and resource group * avoid test racing * fix test * add sample * add resource id test
SUMMARY
Fixes #37917
Fixes #37734
ISSUE TYPE
COMPONENT NAME
azure_rm_networkinterface
ANSIBLE VERSION
ADDITIONAL INFORMATION