Skip to content

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

Merged
Juliehzl merged 12 commits intoAzure:devfrom
Bhuvaneswari-Santharam:azsauthapi
Aug 6, 2021
Merged

{Azurestack} ACR & AKS: Adding Tests and Fixing Authorization api-version for hybrid profile#18443
Juliehzl merged 12 commits intoAzure:devfrom
Bhuvaneswari-Santharam:azsauthapi

Conversation

@Bhuvaneswari-Santharam
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

yonzhan commented Jun 11, 2021

Azurestack

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

Juliehzl commented Jun 17, 2021

please make CI pass first

@Bhuvaneswari-Santharam
Copy link
Copy Markdown
Member Author

Bhuvaneswari-Santharam commented Jun 23, 2021

@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
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
Copy Markdown
Contributor

@Juliehzl Juliehzl Jun 24, 2021

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
Copy Markdown
Member

@bganapa bganapa Jun 24, 2021

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
Copy Markdown
Contributor

@Juliehzl Juliehzl Jun 25, 2021

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
Copy Markdown
Contributor

@adewaleo adewaleo Jun 30, 2021

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
Copy Markdown
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
Copy Markdown
Collaborator

yonzhan commented Jun 30, 2021

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

@adewaleo
Copy link
Copy Markdown
Contributor

adewaleo commented Jun 30, 2021

@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 requested changes Jul 1, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@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
Copy Markdown
Contributor

@Juliehzl Juliehzl Aug 6, 2021

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
Copy Markdown
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 requested changes Aug 6, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@Juliehzl Juliehzl Aug 6, 2021

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

@Juliehzl Juliehzl Juliehzl approved these changes

@FumingZhang FumingZhang FumingZhang approved these changes

@andyliuliming andyliuliming Awaiting requested review from andyliuliming

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys

@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

@kairu-ms kairu-ms Awaiting requested review from kairu-ms

@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

+2 more reviewers

@bganapa bganapa bganapa approved these changes

@adewaleo adewaleo adewaleo approved these changes

Reviewers whose approvals may not affect merge requirements

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