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.

Sign up for GitHub

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

Jump to bottom

[SQL] az sql instance-pool: Add instance pools command group #11721

Merged
merged 141 commits into from
Apr 28, 2020
Merged

[SQL] az sql instance-pool: Add instance pools command group #11721

merged 141 commits into from
Apr 28, 2020

Conversation

rohands
Copy link
Contributor

@rohands rohands commented Dec 31, 2019

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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@rohands rohands requested a review from jaredmoo as a code owner December 31, 2019 00:21
@Juliehzl Juliehzl self-assigned this Dec 31, 2019
@Juliehzl Juliehzl self-requested a review December 31, 2019 01:52
@yonzhan yonzhan added this to the S163 milestone Dec 31, 2019
@yonzhan
Copy link
Collaborator

yonzhan commented Dec 31, 2019

@Juliehzl please help with the PR.

@yonzhan yonzhan requested a review from zhoxing-ms December 31, 2019 02:45
@Juliehzl
Copy link
Contributor

please locally try azdev test test_sql_instance_pool --live --debug to see more information for your test error. According to the log, it looks that there are required arguments missed.

Because you mark your test as record_only, --live will be skipped when you already have recording file. please remove them and run azdev test test_sql_instance_pool --live --debug

@Juliehzl
Copy link
Contributor

Hi @rohands, please also update history.rst file for your new features supported.

@Juliehzl Juliehzl modified the milestones: S163, S164 Jan 3, 2020
@rohands
Copy link
Contributor Author

rohands commented Jan 7, 2020

please locally try azdev test test_sql_instance_pool --live --debug to see more information for your test error. According to the log, it looks that there are required arguments missed.

Because you mark your test as record_only, --live will be skipped when you already have recording file. please remove them and run azdev test test_sql_instance_pool --live --debug

@rohands rohands closed this Jan 7, 2020
@rohands rohands reopened this Jan 7, 2020
@rohands
Copy link
Contributor Author

rohands commented Jan 7, 2020

Hi @rohands, please also update history.rst file for your new features supported.

Added

Juliehzl
Juliehzl reviewed Jan 10, 2020
View reviewed changes
src/azure-cli/azure/cli/command_modules/sql/_params.py Outdated
'tier',
arg_type=tier_param_type,
required=True,
help='The edition component of the sku. Allowed values: GeneralPurpose, BusinessCritical.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use get_enum_type() here and delete " Allowed values: GeneralPurpose, BusinessCritical."

Juliehzl
Juliehzl reviewed Jan 10, 2020
View reviewed changes
src/azure-cli/azure/cli/command_modules/sql/_params.py
c.argument('family',
arg_type=family_param_type,
required=True,
help='The compute generation component of the sku. '
Copy link
Contributor

Choose a reason for hiding this comment

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

The same as above. Please use get_enum_type()

@yonzhan yonzhan modified the milestones: S168, S169 - For Build Apr 14, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 14, 2020

@rohands OK, move to next Sprint currently.

Juliehzl
Juliehzl reviewed Apr 21, 2020
View reviewed changes
src/azure-cli/HISTORY.rst Outdated
@@ -6,6 +6,11 @@ Release History
2.3.1
++++++

**SQL**
Copy link
Contributor

Choose a reason for hiding this comment

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

This part should be removed and add it in description.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change history.rst because we will generate it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, where is the description section?

Copy link
Contributor

Choose a reason for hiding this comment

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

#11721 (comment) or title

Juliehzl
Juliehzl reviewed Apr 21, 2020
View reviewed changes
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py Outdated
expect_failure=True)

# test show sql managed instance doesn't return anything
self.cmd('sql instance-pool show -g {} -n {}'
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use list to count instance number.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we need a test case with list command and check the amount of array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Included this

@rohands
Copy link
Contributor Author

rohands commented Apr 22, 2020 •
edited
Loading

image

Not sure why CLI tests are failing as a apart of automation. azdev test sql is successful locally.
After some investigation, I see this is the error log -

No match for the request (<Request (GET) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/billingPools/providers/Microsoft.Network/virtualNetworks/vnet-billingPool1/subnets/InstancePool?api-version=2020-03-01>) was found.
Found 1 similar requests with 1 different matcher(s) :

1 - (<Request (GET) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/billingPools/providers/Microsoft.Network/virtualNetworks/vnet-billingPool1/subnets/InstancePool?api-version=2019-11-01>).
Matchers succeeded : ['method', 'scheme', 'host', 'port', 'path']
Matchers failed :
_custom_request_query_matcher - assertion failure :
None

Do I need to update my version and re-record the tests?
I am using the same version - azure-mgmt-sql==0.18.0

@yonzhan
Copy link
Collaborator

yonzhan commented Apr 22, 2020

add to S169

Juliehzl
Juliehzl suggested changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@Juliehzl Juliehzl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. Still no list command in test.
  2. Please fix style using azdev style sql
  3. Please fix test for network by syncing with latest dev

@rohands
Copy link
Contributor Author

rohands commented Apr 24, 2020

  1. Still no list command in test.
    I have included list command in tests.
  1. Please fix style using azdev style sql
    Style is fixed
  1. Please fix test for network by syncing with latest dev
    Will do

Juliehzl
Juliehzl reviewed Apr 26, 2020
View reviewed changes
src/azure-cli/azure/cli/command_modules/sql/tests/latest/test_sql_commands.py Outdated
JMESPathCheck('resourceGroup', resource_group),
JMESPathCheck('tags', {})])

self.cmd('sql instance-pool list', checks=[self.greater_than('length(@)', 1)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check with a accurate value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean why not use accurate value here, instead of greater_than(). Is there any concern?

@Juliehzl Juliehzl merged commit 015be59 into Azure:dev Apr 28, 2020
@rohands rohands deleted the cli_for_ip branch April 28, 2020 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weizeng-msft weizeng-msft weizeng-msft left review comments

@Juliehzl Juliehzl Juliehzl approved these changes

@jaredmoo jaredmoo jaredmoo approved these changes

@zhoxing-ms zhoxing-ms Awaiting requested review from zhoxing-ms

Assignees

@Juliehzl Juliehzl

Labels
do-not-merge feature-request Service Attention This issue is responsible by Azure service team.
Projects
None yet
Milestone
S169 - For Build
Development

Successfully merging this pull request may close these issues.