-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Knack conversion for lab module #5068
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5068 |
opened an issue for the lab tests: |
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.
Overall LGTM. A few changes requested.
import azure.cli.command_modules.lab._help # pylint: disable=unused-import | ||
|
||
|
||
def load_params(_): | ||
import azure.cli.command_modules.lab._params # pylint: disable=redefined-outer-name, unused-variable | ||
class LabCommandsLoader(AzCommandsLoader): |
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.
Recommend DevTestLabCommandsLoader
from collections import OrderedDict | ||
|
||
|
||
def export_artifacts(formula): |
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.
Please manually test these to make sure they work correctly.
c.argument('artifacts', type=get_json_object) | ||
# Image related arguments | ||
for arg_name in ['os_type', 'gallery_image_reference', 'custom_image_id']: | ||
c.ignore(arg_name) |
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.
Please replace this with:
c.ignore('os_type', 'gallery_image_reference', 'custom_image_id')
c.ignore('network_interface') | ||
for arg_name in ['lab_subnet_name', 'lab_virtual_network_id', 'disallow_public_ip_address', | ||
'network_interface']: | ||
c.ignore(arg_name) |
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.
Same here...
g.command('start', 'start') | ||
g.command('stop', 'stop') | ||
g.command('apply-artifacts', 'apply_artifacts') | ||
g.custom_command('list', 'list_vm', client_factory=get_devtestlabs_virtual_machine_operation, |
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.
This should also work:
with self.command_group('lab vm', virtual_machine_operations, client_factory=get_devtestlabs_virtual_machine_operations) as g:
If it doesn't... I'll make it work!
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.
This will only work if the command_type does not have the kwarg('client_factory') specified.
Right now any kwargs in the <custom_>command_type will override the ones in the group kwargs:
https://github.com/Azure/azure-cli/blob/KnackConversion/src/azure-cli-core/azure/cli/core/commands/__init__.py#L689-L694
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.
@tjprescott once the above issue is fixed, i.e:
(command kwargs) overrides => (group kwargs) overrides => (command-type kwargs)
We can fix #5072
By just saying, kwargs.get('client_factory')
in _cli_command()
, instead of looking into the command_type again.
@ResourceGroupPreparer(name_prefix='cliautomation') | ||
def test_lab_gallery_vm_mgmt(self, resource_group): | ||
self.kwargs.update({ | ||
'resource_group': 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 ResourceGroupPreparer automatically populates kwargs with this value under 'rg'. Please update accordingly.
@tjprescott addressed your comments |
-couldnt re-record tests #5069
-fixed issue with AzCommandsLoader with client_factory kwarg precedence
Closes #4937