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
adding possibility to specify resource group for referred virtual net… #36768
adding possibility to specify resource group for referred virtual net… #36768
Conversation
The test
The test
The test
|
The test
|
@@ -425,7 +432,10 @@ def exec_module(self, **kwargs): | |||
# parse the virtual network resource group and name | |||
virtual_network_dict = parse_resource_id(self.virtual_network_name) | |||
virtual_network_name = virtual_network_dict.get('name') | |||
virtual_network_resource_group = virtual_network_dict.get('resource_group', self.resource_group) |
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.
The origin logic is
dict = parse_virtual_network_id()
virtual_network_resource_group = dict[resource_group] || self.resource_group
And now it is changed to
dict = parse_virtual_network_id()
virtual_network_resource_group = dict[resource_group] || self.virtual_network_resource_group || self.resource_group
I wonder why this extra logic is needed? Parsing id shares the same behavior like Azure-CLI
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.
dict[resource_group] will work if user specifies entire path/id of virtual_network, but if they want to specify virtual_network from another resource group, specifying virtual_network_resource_group will be simplified way to do this.
This pattern is also implemented in azure_rm_virtualmachine.
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.
It is okay, here I would like to make the precedence more clearly.
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.
I agree, the documentation should be clearer around the precedence, i.e. virtual_network_resource_group
overrides resource_group
and so on.
ready_for_review |
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.
Just a minor nitpick around documentation and making the precedence clearer. I would also like to see a test to cover this feature to ensure no regressions occur in the future.
@@ -425,7 +432,10 @@ def exec_module(self, **kwargs): | |||
# parse the virtual network resource group and name | |||
virtual_network_dict = parse_resource_id(self.virtual_network_name) | |||
virtual_network_name = virtual_network_dict.get('name') | |||
virtual_network_resource_group = virtual_network_dict.get('resource_group', self.resource_group) |
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.
I agree, the documentation should be clearer around the precedence, i.e. virtual_network_resource_group
overrides resource_group
and so on.
@jborean93 , could you please review this change and see whether it is ok to move forward for PR merge? It also passed the test of the submitter @ingokofler. Thank you. |
@zikalino sorry it seems like it has some branch conflicts, are you able to fix them up and I'll get onto the review straight away. |
@jborean93 -- resolved |
@zikalino this PR contains the following merge commits: Please rebase your branch to remove these commits. |
oh no... GitHub created merge commit :-( |
aa412d9
to
f45def1
Compare
ok, now should be rebased |
rebuild_merge |
@jborean93 seems like this one is not merged automatically either.... |
…work
SUMMARY
Fixes #36624
ISSUE TYPE
COMPONENT NAME
azure_rm_networkinterface
ANSIBLE VERSION
2.4
ADDITIONAL INFORMATION