Migrate Azure to ARM #63
Conversation
This accepts the json cred fiel directly which includes the proper resource management endpoint for each service principal.
Supplying subnet-id vnet-name and vnet-resource-group launches instance into the given subnet. All created resources will reside in a new resource group for easy cleanup when testing is finished.
Required by all azure packages but explicitly imported for client factory function.
Extends from ValueError. Thus catch ValueError instead.
smarlowucf
force-pushed
the
azure-arm
branch
from
04ea0da
to
7491206
Compare
|
Unit tests updated. |
ipa/ipa_azure.py
Outdated
| Attach NIC to the subnet and public IP provided. | ||
| """ | ||
| try: | ||
| nic_operation = self.network.network_interfaces.create_or_update( |
There was a problem hiding this comment.
nic_setup or nic_config, nic_operation is a weird name for what is happening here.
There was a problem hiding this comment.
Yeah, it is although the return value from Azure is an operation type.
There was a problem hiding this comment.
I understand but we do not name things by type, we don't usually use repo_list, but would call it repositories, for example
There was a problem hiding this comment.
😄 Right, had trouble thinking of a good name for them.
ipa/ipa_azure.py
Outdated
| 'id': public_ip.id | ||
| }, | ||
| }] | ||
| } |
There was a problem hiding this comment.
I think this dict should be created outside the argument list to the create_or_update() method. It makes the call location more concise and it makes it easier to track the arguments
ipa/ipa_azure.py
Outdated
| Create dynamic public IP address in the resource group. | ||
| """ | ||
| try: | ||
| public_ip_operation = \ |
There was a problem hiding this comment.
naming, see earlier comment
ipa/ipa_azure.py
Outdated
| { | ||
| 'location': region, | ||
| 'public_ip_allocation_method': 'Dynamic' | ||
| } |
ipa/ipa_azure.py
Outdated
| Attempt to create or update VM instance based on vm_parameters config. | ||
| """ | ||
| try: | ||
| vm_operation = self.compute.virtual_machines.create_or_update( |
ipa/ipa_azure.py
Outdated
| Create a subnet in the provided vnet and resource group. | ||
| """ | ||
| try: | ||
| subnet_operation = self.network.subnets.create_or_update( |
ipa/ipa_azure.py
Outdated
| 'address_space': { | ||
| 'address_prefixes': ['10.0.0.0/27'] | ||
| } | ||
| } |
There was a problem hiding this comment.
pre-create the dict and naming ;)
ipa/ipa_azure.py
Outdated
| 'primary': True | ||
| }] | ||
| } | ||
| } |
There was a problem hiding this comment.
There's is really nothing wrong with creating temporary variables. This will make the composition of the information much more digestible.
ipa/ipa_azure.py
Outdated
| instance_name=self.running_instance_id | ||
| ) | ||
| self._wait_on_instance('ReadyRole') | ||
| start_operation.wait() |
Split VM config into separate dictionary components.
smarlowucf
force-pushed
the
azure-arm
branch
from
338386f
to
14bc8a9
Compare
|
@rjschwei Look good? |
Remove azurectl.
@rjschwei This is not ready for merge as tests have not been migrated.
However, code is ready for review. Does this seem like an okay route to allowing subnet arg?
I.e. subnet-id, vnet-name and vnet-resource-group can be supplied to launch the new instance in the provided subnet.
All resources that are created by IPA still go into their own group which makes cleanup very nice. And in testing this worked fine to have a NIC attached to a subnet in a different group.