Skip to content
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

SCVMM Extension for Public Preview #4631

Merged
merged 9 commits into from
Apr 18, 2022
Merged

Conversation

nascarsayan
Copy link
Contributor

@nascarsayan nascarsayan commented Apr 5, 2022


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@nascarsayan nascarsayan marked this pull request as ready for review April 5, 2022 13:05
@nascarsayan nascarsayan closed this Apr 5, 2022
@nascarsayan nascarsayan reopened this Apr 5, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 5, 2022

SCVMM

@yonzhan yonzhan requested review from kairu-ms and jsntcy April 5, 2022 14:03
@yonzhan yonzhan requested a review from necusjz April 5, 2022 14:03
@yonzhan yonzhan added this to the Apr 2022 (2022-04-26) milestone Apr 5, 2022
@nascarsayan
Copy link
Contributor Author

nascarsayan commented Apr 5, 2022

Hi @kairu-ms , the build is failing with this error:

ModuleNotFoundError: No module named 'azure_devtools'

Is it some issue with the PR?

python -m unittest discover -v <path-to-repo>/src/scvmm is passing locally
image

@nascarsayan
Copy link
Contributor Author

nascarsayan commented Apr 5, 2022

I understood the issue. It was with this line:
from azure_devtools.scenario_tests import AllowLargeResponse
I have removed this line, it should work now.

In my local environment from azure.cli.testsdk.scenario_tests import AllowLargeResponse is not working.

@subbartt
Copy link

@kairu-ms , we are targeting to publish the az cli by end of April, let us know if there is any feedback so that we can incorporate.

Comment on lines +29 to +31
g.custom_command('connect', 'connect_vmmserver', supports_no_wait=True)
g.custom_command('update', 'update_vmmserver', supports_no_wait=True)
g.custom_command('delete', 'delete_vmmserver', supports_no_wait=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "wait" commands for this command group. For example:

Copy link
Contributor Author

@nascarsayan nascarsayan Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added wait command everywhere. I had to change the scvmm vm-template wait command to custom_wait_command, because --virtual-machine-template-name parameter was exceeding the max length of parameter (azdev linter was failing). Any suggestions if we could rename the parameter in the wait command without using custom wait?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nascarsayan, you can suppress the max length of parameter linter validation by adding a linter_exclusions.yml file. For example:

datafactory get-git-hub-access-token:
parameters:
git_hub_access_token_base_url:
rule_exclusions:
- option_length_too_long

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nascarsayan, another way is that you can use options_list property of argument to provide a short name instead of --virtual-machine-template-name, for example:

c.argument('factory_vsts_configuration', options_list=['--vsts-config', '--factory-vsts-configuration'],
action=AddFactoryVstsConfiguration, nargs='+', help='Factory\'s VSTS repo information.',
arg_group='RepoConfiguration')

Copy link
Contributor Author

@nascarsayan nascarsayan Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kairu-ms, I've used options_list to use a shorter name for the param. Thanks for the references.

One question: We don't have a wait on a particular disk/nic of the VM, is that fine? The wait we are providing for scvmm vm disk / scvmm vm nic is the same as for scvmm vm.
These command groups use VirtualMachinesOperations under the hood, i.e., there's no separate client_factory for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that’s ok

@kairu-ms
Copy link
Contributor

@kairu-ms , we are targeting to publish the az cli by end of April, let us know if there is any feedback so that we can incorporate.

Please ping me when above comments resolved. I will help you to release extension on time.

@kairu-ms kairu-ms merged commit 7cda730 into Azure:main Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants