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] Add sovereign cloud support for Storage #868

Merged
merged 4 commits into from
Apr 16, 2021

Conversation

davidkpiano
Copy link
Contributor

This PR resolves the Base URI for the Resource Group module to support sovereign cloud.

Addresses #811, fixes #861

cc. @rguthriemsft

@hattan
Copy link
Contributor

hattan commented Apr 13, 2021

[Microsoft CI Bot] TL;DR; success 👍

You can check the status of the CI Pipeline logs here ; https://github.com/aztfmod/terratest/actions/runs/745238923

Copy link
Contributor

@HadwaAbdelhalem HadwaAbdelhalem left a comment

Choose a reason for hiding this comment

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

Thanks @davidkpiano , one last NIT change

@@ -191,6 +192,7 @@ func GetStorageAccountClientE(subscriptionID string) (*storage.AccountsClient, e
}

// GetStorageBlobContainerClientE creates a storage container client.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the method GetStorageBlobContainerClientE as it is no longer used

Copy link
Contributor

@yorinasub17 yorinasub17 Apr 16, 2021

Choose a reason for hiding this comment

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

I mentioned this somewhere, but I can't find where I said this, so reiterating it here:

Since this is a public method, removing it means we will have to cut a backward incompatible release. It would be a bit of a pain for us to release a backward incompatible release for each of these changes. I think it would be better to keep these in so that we can merge all of these changes as backward compatible patch releases, and then at the very end once all the files are updated, we can have one big PR to remove all the Get.*Client functions and cut one backward incompatible release.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good @yorinasub17 , I approved it and it is ready for review @gruntwork-terratest-terragrunt

@@ -174,6 +174,7 @@ func GetStorageAccountPropertyE(storageAccountName, resourceGroupName, subscript
}

// GetStorageAccountClientE creates a storage account client.
// TODO: remove in next version

This comment was marked as resolved.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM as well! Going to merge and release this version.

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.

[Azure] Add sovereign cloud support for storage
5 participants