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

Azure - Migrate snapshot to a resource group #1019

Merged
merged 5 commits into from
Jun 16, 2021
Merged

Azure - Migrate snapshot to a resource group #1019

merged 5 commits into from
Jun 16, 2021

Conversation

bathina2
Copy link
Contributor

@bathina2 bathina2 commented Jun 9, 2021

Change Overview

The following PR enables Azure to accept a resource group to which a newly create snapshot can belong.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • πŸ› Bugfix
  • 🌻 Feature
  • πŸ—ΊοΈ Documentation
  • πŸ€– Test

Issues

  • K10-7358

Test Plan

  • πŸ’ͺ Manual
  • ⚑ Unit test
  • πŸ’š E2E

@@ -225,7 +225,12 @@ func (s *AdStorage) SnapshotCopyWithArgs(ctx context.Context, from blockstorage.
},
}

result, err := s.azCli.SnapshotsClient.CreateOrUpdate(ctx, s.azCli.ResourceGroup, snapName, createSnap)
migrateResourceGroup := s.azCli.ResourceGroup
if val, ok := args[blockstorage.AzureMigrateStorageKey]; ok && val != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be AzureMigrateResourceGroup instead of AzureMigrateStorageKey ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are very correct

@bathina2 bathina2 changed the title Migrateresourcegroup Azure - Migrate snapshot to a resource group Jun 11, 2021
Comment on lines +229 to +231
if val, ok := args[blockstorage.AzureMigrateResourceGroup]; ok && val != "" {
migrateResourceGroup = val
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add to the function doc comment to describe this key.

@@ -36,6 +36,7 @@ const (
AzureResurceMgrEndpoint = "AZURE_RESOURCE_MANAGER_ENDPOINT"
AzureMigrateStorageAccount = "AZURE_MIGRATE_STORAGE_ACCOUNT_NAME"
AzureMigrateStorageKey = "AZURE_MIGRATE_STORAGE_ACCOUNT_KEY"
AzureMigrateResourceGroup = "AZURE_MIGRATE_RESOURCE_GROUP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be referenced in pkg/blockstorage/blockstorage_test.go?

Copy link
Contributor

@carlbraganza carlbraganza left a comment

Choose a reason for hiding this comment

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

Missing reference from the existing UT that covers the other Azure env variables.

@bathina2
Copy link
Contributor Author

Missing reference from the existing UT that covers the other Azure env variables.

That test is skipped.

@bathina2 bathina2 added the kueue label Jun 16, 2021
@mergify mergify bot merged commit 5e8e217 into master Jun 16, 2021
@mergify mergify bot deleted the migrateRG branch June 16, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants