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

{Azurestack} ACR & AKS: Adding Tests and Fixing Authorization api-version for hybrid profile #18443

Merged
merged 12 commits into from
Aug 6, 2021
Merged

{Azurestack} ACR & AKS: Adding Tests and Fixing Authorization api-version for hybrid profile #18443

merged 12 commits into from
Aug 6, 2021

Conversation

Bhuvaneswari-Santharam
Copy link
Member

@Bhuvaneswari-Santharam Bhuvaneswari-Santharam commented Jun 10, 2021 •
edited by evelyn-ys
Loading

Description

Testing Guide

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

@bganapa bganapa changed the title [Azurestack][ACR][AKS]Change Authorization api-version for hybrid profile [Azurestack][ACR][AKS] Adding Tests and Fixing Authorization api-version for hybrid profile Jun 10, 2021
@yonzhan yonzhan added this to the S189 milestone Jun 11, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 11, 2021

Azurestack

@Juliehzl Juliehzl mentioned this pull request Jun 15, 2021
Merged
3 tasks
@Juliehzl
Copy link
Contributor

please make CI pass first

@Bhuvaneswari-Santharam
Copy link
Member Author

@Juliehzl - All CI checks passed. Please review

Juliehzl
Juliehzl reviewed Jun 24, 2021
View reviewed changes
src/azure-cli-core/azure/cli/core/profiles/_shared.py
@@ -241,7 +241,7 @@ def default_api_version(self):
ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2016-06-01',
ResourceType.MGMT_RESOURCE_TEMPLATESPECS: '2015-01-01',
ResourceType.MGMT_NETWORK_DNS: '2016-04-01',
ResourceType.MGMT_AUTHORIZATION: SDKProfile('2016-09-01', {
ResourceType.MGMT_AUTHORIZATION: SDKProfile('2015-07-01', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a downgrade for authorization?

Copy link
Member

Choose a reason for hiding this comment

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

This is not a downgrade, This is a bug fix. In our earlier PR we have mistakenly updated the api version @Juliehzl

Juliehzl
Juliehzl reviewed Jun 25, 2021
View reviewed changes
src/azure-cli/azure/cli/command_modules/acr/check_health.py
from ._errors import CMK_MANAGED_IDENTITY_ERROR
_handle_error(CMK_MANAGED_IDENTITY_ERROR.format_error_message(registry_name), ignore_errors)
if cmd.supported_api_version(min_api='2020-11-01-preview', resource_type=ResourceType.MGMT_CONTAINERREGISTRY): # pylint: disable=too-many-nested-blocks
# CMK settings
Copy link
Contributor

Choose a reason for hiding this comment

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

please also add acr code owner to review your change

Copy link
Contributor

Choose a reason for hiding this comment

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

@Bhuvaneswari-Santharam I tested this locally and it looks good. @northtyphoon we should consider adding some unit tests for acr check-health

adewaleo
adewaleo approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@adewaleo adewaleo left a comment

Choose a reason for hiding this comment

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

Approved for ACR.
@Bhuvaneswari-Santharam I tested this locally and it looks good.
@northtyphoon we should consider adding some unit tests for acr check-health

@yonzhan
Copy link
Collaborator

yonzhan commented Jun 30, 2021

@northtyphoon please add more unit tests as @adewaleo mentioned.

@adewaleo
Copy link
Contributor

@northtyphoon please add more unit tests as @adewaleo mentioned.

@yonzhan, I was making this suggestion as a follow up item. Given acr check-health (convenience command for diagnosis) doesn't currently have unit tests. I did manually test to see that this change doesn't break acr check-health. I won't want this request to block @Bhuvaneswari-Santharam

FumingZhang
FumingZhang suggested changes Jul 1, 2021
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

Need to update some recording files, since some of the tests failed.

.../command_modules/acs/tests/hybrid_2020_09_01/recordings/test_aks_availability_zones_msi.yaml Outdated Show resolved Hide resolved
...cs/tests/hybrid_2020_09_01/recordings/test_aks_create_aadv1_and_update_with_managed_aad.yaml Outdated Show resolved Hide resolved
...ests/hybrid_2020_09_01/recordings/test_aks_create_aadv1_and_update_with_managed_aad_msi.yaml Outdated Show resolved Hide resolved
...ure/cli/command_modules/acs/tests/hybrid_2020_09_01/recordings/test_aks_create_blb_vmas.yaml Outdated Show resolved Hide resolved
...cli/command_modules/acs/tests/hybrid_2020_09_01/recordings/test_aks_create_blb_vmas_msi.yaml Outdated Show resolved Hide resolved
...2020_09_01/recordings/test_aks_create_slb_vmss_with_default_mgd_outbound_ip_then_update.yaml Outdated Show resolved Hide resolved
...i/command_modules/acs/tests/hybrid_2020_09_01/recordings/test_aks_create_fqdn_subdomain.yaml Outdated Show resolved Hide resolved
...nd_modules/acs/tests/hybrid_2020_09_01/recordings/test_openshift_create_default_service.yaml Outdated Show resolved Hide resolved
...les/acs/tests/hybrid_2020_09_01/recordings/test_openshift_create_default_service_no_aad.yaml Outdated Show resolved Hide resolved
...nd_modules/acs/tests/hybrid_2020_09_01/recordings/test_openshift_create_service_no_wait.yaml Outdated Show resolved Hide resolved
@yonzhan yonzhan modified the milestones: S189, Jul 2021 (2021-08-03) Jul 2, 2021
@jsntcy jsntcy closed this Jul 26, 2021
@jsntcy jsntcy reopened this Jul 26, 2021
Juliehzl
Juliehzl reviewed Aug 6, 2021
View reviewed changes
src/azure-cli/azure/cli/command_modules/acs/tests/hybrid_2020_09_01/test_acs_client.py Outdated

# pylint: skip-file
import unittest
import mock
Copy link
Contributor

Choose a reason for hiding this comment

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

mock module is deprecated in azure cli. see #19024

FumingZhang
FumingZhang approved these changes Aug 6, 2021
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

AKS test recordings looks good to me.

Juliehzl
Juliehzl suggested changes Aug 6, 2021
View reviewed changes
Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

mock module is deprecated in azure cli. see #19024

Juliehzl
Juliehzl reviewed Aug 6, 2021
View reviewed changes
src/azure-cli/azure/cli/command_modules/acr/tests/hybrid_2020_09_01/test_acr_commands_mock.py Outdated
from urllib import urlencode
import json
import unittest
import mock
Copy link
Contributor

Choose a reason for hiding this comment

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

still have mock here.

@Juliehzl Juliehzl merged commit 2693f38 into Azure:dev Aug 6, 2021
@FumingZhang FumingZhang mentioned this pull request Aug 9, 2021
Merged
3 tasks
@evelyn-ys evelyn-ys changed the title [Azurestack][ACR][AKS] Adding Tests and Fixing Authorization api-version for hybrid profile {Azurestack} ACR & AKS: Adding Tests and Fixing Authorization api-version for hybrid profile Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bganapa bganapa bganapa approved these changes

@FumingZhang FumingZhang FumingZhang approved these changes

@Juliehzl Juliehzl Juliehzl approved these changes

@adewaleo adewaleo adewaleo approved these changes

@andyliuliming andyliuliming Awaiting requested review from andyliuliming

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys evelyn-ys is a code owner

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@houk-ms houk-ms Awaiting requested review from houk-ms

@jiasli jiasli Awaiting requested review from jiasli jiasli is a code owner

@jsntcy jsntcy Awaiting requested review from jsntcy jsntcy is a code owner

@kairu-ms kairu-ms Awaiting requested review from kairu-ms kairu-ms is a code owner

@northtyphoon northtyphoon Awaiting requested review from northtyphoon

@yungezz yungezz Awaiting requested review from yungezz

@zqingqing1 zqingqing1 Awaiting requested review from zqingqing1

@zhoxing-ms zhoxing-ms Awaiting requested review from zhoxing-ms zhoxing-ms is a code owner

Assignees

@zhoxing-ms zhoxing-ms

@Juliehzl Juliehzl

Labels
Azure Stack AzureStack
Projects
None yet
Milestone
Aug 2021 (2021-09-07)
Development

Successfully merging this pull request may close these issues.
8 participants
@Bhuvaneswari-Santharam @yonzhan @Juliehzl @adewaleo @bganapa @FumingZhang @jsntcy @zhoxing-ms