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

Adding SBM support #24116

Merged
merged 8 commits into from
Dec 17, 2024
Merged

Adding SBM support #24116

merged 8 commits into from
Dec 17, 2024

Conversation

ananyabhatia-msft
Copy link

@ananyabhatia-msft ananyabhatia-msft commented Feb 6, 2024

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • For SDK-based development mode, update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • For autorest-based development mode, include the changelog in the PR description.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

Copy link

azure-client-tools-bot-prd bot commented Feb 6, 2024

️✔️Az.Accounts
️✔️Build
️✔️PowerShell Core - Windows
⚠️Az.Migrate
️✔️Build
️✔️PowerShell Core - Windows
️✔️Breaking Change Check
️✔️PowerShell Core - Windows
⚠️Signature Check
⚠️PowerShell Core - Windows
Type Cmdlet Description Remediation
⚠️ Get-AzMigrateHCIJob Get-AzMigrateHCIJob Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateHCIJob Get-AzMigrateHCIJob changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateHCIReplicationFabric Get-AzMigrateHCIReplicationFabric Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateHCIReplicationFabric Get-AzMigrateHCIReplicationFabric changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateHCIServerReplication Get-AzMigrateHCIServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateHCIServerReplication Get-AzMigrateHCIServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateJob Get-AzMigrateJob Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateJob Get-AzMigrateJob changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateProject Get-AzMigrateProject Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateProject Get-AzMigrateProject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateReplicationFabric Get-AzMigrateReplicationFabric Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateReplicationFabric Get-AzMigrateReplicationFabric changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateReplicationPolicy Get-AzMigrateReplicationPolicy Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateReplicationPolicy Get-AzMigrateReplicationPolicy changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateReplicationProtectionContainer Get-AzMigrateReplicationProtectionContainer Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateReplicationProtectionContainer Get-AzMigrateReplicationProtectionContainer changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateReplicationProtectionContainerMapping Get-AzMigrateReplicationProtectionContainerMapping Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateReplicationProtectionContainerMapping Get-AzMigrateReplicationProtectionContainerMapping changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateReplicationRecoveryServicesProvider Get-AzMigrateReplicationRecoveryServicesProvider Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateReplicationRecoveryServicesProvider Get-AzMigrateReplicationRecoveryServicesProvider changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateRunAsAccount Get-AzMigrateRunAsAccount Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateRunAsAccount Get-AzMigrateRunAsAccount changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateServerReplication Get-AzMigrateServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateServerReplication Get-AzMigrateServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateSite Get-AzMigrateSite Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateSite Get-AzMigrateSite changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Get-AzMigrateSolution Get-AzMigrateSolution Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Get-AzMigrateSolution Get-AzMigrateSolution changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ New-AzMigrateDiskMapping New-AzMigrateDiskMapping Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ New-AzMigrateDiskMapping New-AzMigrateDiskMapping changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ New-AzMigrateHCIDiskMappingObject New-AzMigrateHCIDiskMappingObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ New-AzMigrateHCINicMappingObject New-AzMigrateHCINicMappingObject changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ New-AzMigrateNicMapping New-AzMigrateNicMapping Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ New-AzMigrateNicMapping New-AzMigrateNicMapping changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ New-AzMigrateServerReplication New-AzMigrateServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ New-AzMigrateServerReplication New-AzMigrateServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Remove-AzMigrateServerReplication Remove-AzMigrateServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Remove-AzMigrateServerReplication Remove-AzMigrateServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Restart-AzMigrateServerReplication Restart-AzMigrateServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Restart-AzMigrateServerReplication Restart-AzMigrateServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Set-AzMigrateDiskMapping Set-AzMigrateDiskMapping Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Set-AzMigrateDiskMapping Set-AzMigrateDiskMapping changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Set-AzMigrateServerReplication Set-AzMigrateServerReplication Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Set-AzMigrateServerReplication Set-AzMigrateServerReplication changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Start-AzMigrateServerMigration Start-AzMigrateServerMigration Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Start-AzMigrateServerMigration Start-AzMigrateServerMigration changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Start-AzMigrateTestMigration Start-AzMigrateTestMigration Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Start-AzMigrateTestMigration Start-AzMigrateTestMigration changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
⚠️ Start-AzMigrateTestMigrationCleanup Start-AzMigrateTestMigrationCleanup Changes the ConfirmImpact but does not set the SupportsShouldProcess property to true in the cmdlet attribute. Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue
⚠️ Start-AzMigrateTestMigrationCleanup Start-AzMigrateTestMigrationCleanup changes the confirm impact. Please ensure that the change in ConfirmImpact is justified Verify that ConfirmImpact is changed appropriately by the cmdlet. It is very rare for a cmdlet to change the ConfirmImpact.
️✔️Help File Existence Check
️✔️PowerShell Core - Windows
⚠️File Change Check
⚠️PowerShell Core - Windows
Type Cmdlet Description Remediation
⚠️ It is required to update ChangeLog.md if you want to release a new version for Az.Migrate. Add a changelog record under Upcoming Release section with past tense.
️✔️UX Metadata Check
️✔️PowerShell Core - Windows
️✔️Test
️✔️PowerShell Core - Linux
️✔️PowerShell Core - MacOS
️✔️PowerShell Core - Windows

Copy link

github-actions bot commented Feb 6, 2024

‼️ DO NOT MERGE THIS PR ‼️
This PR was labeled "Do Not Merge" because it contains code change that cannot be merged. Please contact the reviewer for more information.

@msJinLei
Copy link
Contributor

@ananyabhatia-msft please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@ananyabhatia-msft Please sign the agreement

Copy link
Contributor

@msJinLei msJinLei left a comment

Choose a reason for hiding this comment

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

Please add the test for your changes

Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

@ananyabhatia-msft
Copy link
Author

@ananyabhatia-msft please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@YanaXu
Copy link
Contributor

YanaXu commented Apr 24, 2024

Hi @ananyabhatia-msft , please join your github account to Azure org.

Copy link
Contributor

@YanaXu YanaXu left a comment

Choose a reason for hiding this comment

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

Could you please add tests for your update? It's hard to tell from the change if it's OK.

Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

@ananyabhatia-msft
Copy link
Author

Could you please add tests for your update? It's hard to tell from the change if it's OK.

The test are already in src/Migrate/Migrate.Autorest/test/Initialize-AzMigrateReplicationInfrastructure.Tests.ps1

@YanaXu
Copy link
Contributor

YanaXu commented Apr 29, 2024

Hi @ananyabhatia-msft , please join the GitHub org as this commet.
And could you update the test to remove "LiveOnly"? "LiveOnly" tests will not be run in CI checks. That means we can't see if it's OK or not.

@ananyabhatia-msft
Copy link
Author

Hi @ananyabhatia-msft , please join the GitHub org as this commet. And could you update the test to remove "LiveOnly"? "LiveOnly" tests will not be run in CI checks. That means we can't see if it's OK or not.

Hi
We were instructed by a reveiwer to add the LiveOnly flag, since recording based tests were not allowed for custom cmdlets
image

Here is the link to post where you can find more details about the discussion.

[string]$objectId,
[string]$saId) {

$storageBlobDataContributorRoleDefinitionId = "ba92f5b4-2d11-453d-a403-e96b0029c9fe"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a hardcoded id. Could you explain more about this?

Copy link
Author

Choose a reason for hiding this comment

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

This is id of the role definition (Storage Blob Data Contributor), taken from the azure documentation: link

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 the ids are defined in 'src/Migrate/Migrate.Autorest/custom/Helper/AzStackHCICommonSettings.ps1'. Please refer to this ps1 file like this.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@YanaXu
Copy link
Contributor

YanaXu commented May 8, 2024

Hi @ananyabhatia-msft , please join the GitHub org as this commet. And could you update the test to remove "LiveOnly"? "LiveOnly" tests will not be run in CI checks. That means we can't see if it's OK or not.

Hi We were instructed by a reveiwer to add the LiveOnly flag, since recording based tests were not allowed for custom cmdlets image

Here is the link to post where you can find more details about the discussion.

Hi @ananyabhatia-msft, it's due to the limiation of test workflow. If so, could you at least provide a screenshot of your live test?

@ananyabhatia-msft
Copy link
Author

Hi @ananyabhatia-msft , please join the GitHub org as this commet. And could you update the test to remove "LiveOnly"? "LiveOnly" tests will not be run in CI checks. That means we can't see if it's OK or not.

Hi We were instructed by a reveiwer to add the LiveOnly flag, since recording based tests were not allowed for custom cmdlets image
Here is the link to post where you can find more details about the discussion.

Hi @ananyabhatia-msft, it's due to the limiation of test workflow. If so, could you at least provide a screenshot of your live test?

Hi @YanaXu
Attaching the screenshots for successful runs for both Public endpoint and Private endpoint scenarios.
Public:
image
Private:
image

@YanaXu
Copy link
Contributor

YanaXu commented Dec 11, 2024

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@YanaXu
Copy link
Contributor

YanaXu commented Dec 16, 2024

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@singhabh27 singhabh27 removed the request for review from prsadhu-ms-idc December 16, 2024 04:35
@YanaXu YanaXu dismissed msJinLei’s stale review December 17, 2024 06:19

confirmed with reviewer and dismiss.

@YanaXu YanaXu merged commit 0f01642 into Azure:generation Dec 17, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants