Skip to content

Conversation

@milanbrkic-ms
Copy link
Member

@milanbrkic-ms milanbrkic-ms commented Oct 5, 2022

Related command

Description

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change
[Component Name 2] az command b: Add some customer-facing feature


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

@yonzhan yonzhan added this to the Oct 2022 (2022-11-01) milestone Oct 5, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Oct 6, 2022

SQL

@milanbrkic-ms milanbrkic-ms changed the title Mibrkic/identity [SQL] az sql midb log-replay start: Add --storage-identity param Oct 7, 2022
@milanbrkic-ms milanbrkic-ms marked this pull request as ready for review October 7, 2022 09:46
@milanbrkic-ms milanbrkic-ms changed the title [SQL] az sql midb log-replay start: Add --storage-identity param [SQL] az sql midb log-replay start: Add --storage-identity parameter Oct 7, 2022
@milanbrkic-ms
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 24105 in repo Azure/azure-cli

@milanbrkic-ms
Copy link
Member Author

@evelyn-ys can you help with review? Thanks

@ResourceGroupPreparer(name_prefix='clitest-HSEP', location='eastus2')
@SqlServerPreparer(name_prefix='clitest-HSEP', location='eastus2')
@AllowLargeResponse()
@live_only() # Could not find tier Hyperscale. Supported tiers are: ['Standard', 'Premium', 'Basic', 'GeneralPurpose', 'BusinessCritical']
Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking change that Hyperscale not supported any more in azure-mgmt-sql==4.0.0b4?

Copy link
Member Author

Choose a reason for hiding this comment

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

I spoke with engineer who implemented this, and she said that Hyperscale type is still not publicly available. She confirmed that I can skip the test

@ResourceGroupPreparer(location='eastus')
@SqlServerPreparer(location='eastus')
@KeyVaultPreparer(location='eastus', name_prefix='sqltdebyok')
@live_only() # User tried to log in to a device from a platform (Unknown) that's currently not supported through Conditional Access policy. Supported device platforms are: iOS, Android, Mac, and Windows flavors.
Copy link
Member

Choose a reason for hiding this comment

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

Did you run this test locally or in some agent? The error msg is rarely seen. I hope this test can be kept as it can pass all previous bumping versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am running all tests from Powershell 7.

Copy link
Member Author

Choose a reason for hiding this comment

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

@evelyn-ys any idea for this issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Test is failing on keyvault command, so I do not think my changes could break command in different module.

Copy link
Member

Choose a reason for hiding this comment

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

Which keyvault command did it fail in? I have never met User tried to log in to a device from a platform (Unknown) that's currently not supported error before... Why would this test using conditional access policy😂

Copy link
Member

Choose a reason for hiding this comment

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

This PR is ready for merge. Do you want me to merge it directly or figure out the root cause of keyvault failure then merge after fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in KeyVaultPreparer if I am reading it correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is ready for merge. Do you want me to merge it directly or figure out the root cause of keyvault failure then merge after fix?

How do you prefer? I am fine with both, just to make it in the next release

Copy link
Member

@evelyn-ys evelyn-ys left a comment

Choose a reason for hiding this comment

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

The code LGTM but I just want that those tests marked as live_only can be fixed in case there's any unexpected breaking change during bumping 4.0.0b4

@evelyn-ys
Copy link
Member

Sorry I have to move this PR to next release milestone

@milanbrkic-ms
Copy link
Member Author

@evelyn-ys pls review again

@milanbrkic-ms
Copy link
Member Author

@evelyn-ys can you please merge this PR?

@evelyn-ys evelyn-ys closed this Oct 18, 2022
@evelyn-ys evelyn-ys reopened this Oct 18, 2022
@evelyn-ys
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@evelyn-ys
Copy link
Member

@wangzelin007 Can you help merge this PR?

@evelyn-ys evelyn-ys merged commit c36f716 into Azure:dev Oct 18, 2022
avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023
…ter (Azure#24105)

* bump version

* add new parameter

* record failing tests

* more tests rec

* more fixing

* put corrrect mask

* one more

* zone tests recorded on eastus

Co-authored-by: Milan Brkic <[email protected]>
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.

6 participants