Navigation Menu

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

New resource azurerm_storage_sync_cloud_endpoint #8540

Merged
merged 20 commits into from Dec 9, 2020

Conversation

yupwei68
Copy link
Contributor

Fix: #2315.

Extend: #7843

Totally there will be three resources:
azurerm_storage_sync
azurerm_storage_sync_group
azurerm_cloud_endpoint

=== RUN TestAccAzureRMCloudEndpoint_basic
=== PAUSE TestAccAzureRMCloudEndpoint_basic
=== CONT TestAccAzureRMCloudEndpoint_basic
--- PASS: TestAccAzureRMCloudEndpoint_basic (390.42s)
=== RUN TestAccAzureRMCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMCloudEndpoint_requiresImport
=== CONT TestAccAzureRMCloudEndpoint_requiresImport
--- PASS: TestAccAzureRMCloudEndpoint_requiresImport (400.43s)

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @yupwei68

Thanks for this PR - I've taken a look through and left some comments inline - if we can fix those up then we should be able to take another look through here.

Thanks!

azurerm/internal/services/storage/client/client.go Outdated Show resolved Hide resolved
azurerm/internal/services/storage/client/client.go Outdated Show resolved Hide resolved
azurerm/internal/services/storage/parsers/id.go Outdated Show resolved Hide resolved
website/docs/r/storage_sync_cloud_endpoint.html.markdown Outdated Show resolved Hide resolved
storage_account_name = azurerm_storage_account.example.name
lifecycle {
ignore_changes = [
acl, metadata
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't be shipping this in examples - what extra configuration is necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There'll be a storage sync signature assigned to the metadata, which we could not get from the storage sync service. Could we just ignore the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And as for acl, it's assigned to the following when a cloud endpoint is created, However, in share definition, start and expiry is necessary and no empty string requested:

 acl {
    id = "GhostedRecall"
    access_policy {
      permissions = "r"
    }
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with tom, if we are ignoring changes we need to fix the resources so its not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Create a PR for the fix #8811

website/docs/r/storage_sync_cloud_endpoint.html.markdown Outdated Show resolved Hide resolved
@yupwei68
Copy link
Contributor Author

Hi @tombuildsstuff Sorry for the mistakes made. Please continue reviewing.

@ghost ghost removed the waiting-response label Sep 24, 2020
@yupwei68
Copy link
Contributor Author

yupwei68 commented Oct 9, 2020

Dependent on #8811.
=== RUN TestAccAzureRMStorageSyncCloudEndpoint_basic
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_basic
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_basic
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_basic (395.54s)
=== RUN TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_requiresImport (411.75s)

@ghost ghost removed the waiting-response label Oct 9, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @yupwei68 - this looks good now, however it seems like some tests are failing:

=== CONT  TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
    TestAccAzureRMStorageSyncCloudEndpoint_requiresImport: testing.go:684: Step 0 error: errors during apply:
        
        Error: waiting for Storage Sync Cloud Endpoint "acctest-CEP-201026174017620407" to be created: Code="MgmtStorageAccountAuthorizationFailed" Message="Unable to read specified storage account. Please check the permissions and try again after some time."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test115906221/main.tf line 38:
          (source code not available)
        
        
    TestAccAzureRMStorageSyncCloudEndpoint_basic: testing.go:684: Step 0 error: errors during apply:
        
        Error: waiting for Storage Sync Cloud Endpoint "acctest-CEP-201026174017617273" to be created: Code="MgmtStorageAccountAuthorizationFailed" Message="Unable to read specified storage account. Please check the permissions and try again after some time."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test146444038/main.tf line 38:
          (source code not available)
        
        
--- FAIL: TestAccAzureRMStorageSyncCloudEndpoint_requiresImport (751.91s)

@yupwei68 yupwei68 requested a review from katbyte October 29, 2020 02:39
@yupwei68
Copy link
Contributor Author

@katbyte Thanks for the comment. Sorry that I can not reproduce this error. Would you mind retrying?

=== RUN TestAccAzureRMStorageSyncCloudEndpoint_basic
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_basic
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_basic
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_basic (310.43s)
=== RUN TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_requiresImport (327.15s)

@yupwei68
Copy link
Contributor Author

yupwei68 commented Nov 6, 2020

Hi @katbyte I've tried several times, I could not reproduce this error. Would you mind testing it again?

@katbyte
Copy link
Collaborator

katbyte commented Nov 6, 2020

I'm still getting this error 🤷‍♀️

[TestAccAzureRMStorageSyncCloudEndpoint_requiresImport] [Test Output]
=== RUN   TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== CONT  TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
    TestAccAzureRMStorageSyncCloudEndpoint_requiresImport: testing.go:684: Step 0 error: errors during apply:
        
        Error: waiting for Storage Sync Cloud Endpoint "acctest-CEP-201106181015426014" to be created: Code="MgmtStorageAccountAuthorizationFailed" Message="Unable to read specified storage account. Please check the permissions and try again after some time."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test285242175/main.tf line 38:
          (source code not available)
        
        
    TestAccAzureRMStorageSyncCloudEndpoint_basic: testing.go:684: Step 0 error: errors during apply:
        
        Error: waiting for Storage Sync Cloud Endpoint "acctest-CEP-201106181015420295" to be created: Code="MgmtStorageAccountAuthorizationFailed" Message="Unable to read specified storage account. Please check the permissions and try again after some time."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test170012512/main.tf line 38:
          (source code not available)
        

you can see this in TC

@katbyte
Copy link
Collaborator

katbyte commented Nov 9, 2020

Thanks @yupwei68 - closer now! only one failure test, and its a different error:

Test Failed

------- Stdout: -------
=== RUN   TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== CONT  TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
    TestAccAzureRMStorageSyncCloudEndpoint_requiresImport: testing.go:684: Step 0 error: errors during apply:
        
        Error: authorization.RoleAssignmentsClient#Get: Failure responding to request: StatusCode=404 -- Original Error: autorest/azure: Service returned an error. Status=404 Code="RoleAssignmentNotFound" Message="The role assignment 'd415abb9-cfec-579e-1469-4d41713e254e' is not found."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test370194010/m

@yupwei68
Copy link
Contributor Author

Thanks @katbyte . There is a role assignment, which is necessary if it's the first time your use "storage sync". I have added this into doc here. This could not be added into acctest, cause it'll fail if the role assignment already exists.
I assume acctests have passed in TC.
image

@yupwei68
Copy link
Contributor Author

I've merged the upstream master

@yupwei68
Copy link
Contributor Author

image

@yupwei68
Copy link
Contributor Author

Thanks for comments @katbyte . Please continue reviewing.

=== RUN TestAccAzureRMStorageSyncCloudEndpoint_basic
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_basic
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_basic
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_basic (302.73s)
=== RUN TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== PAUSE TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
=== CONT TestAccAzureRMStorageSyncCloudEndpoint_requiresImport
--- PASS: TestAccAzureRMStorageSyncCloudEndpoint_requiresImport (313.50s)

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @yupwei68 - LGTM 👍

@katbyte
Copy link
Collaborator

katbyte commented Dec 8, 2020

@yupwei68 - once we fix the merge conflict this should be good to merge

@yupwei68
Copy link
Contributor Author

yupwei68 commented Dec 9, 2020

Thanks for your comments @katbyte . Tests have passed. Please continue reviewing.
image

@yupwei68 yupwei68 requested a review from katbyte December 9, 2020 03:08
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

THanks

@katbyte katbyte added this to the v2.40.0 milestone Dec 9, 2020
@katbyte katbyte merged commit 7ac76b8 into hashicorp:master Dec 9, 2020
katbyte added a commit that referenced this pull request Dec 9, 2020
@ghost
Copy link

ghost commented Dec 10, 2020

This has been released in version 2.40.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.40.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jan 9, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Jan 9, 2021
@yupwei68 yupwei68 deleted the wyp-sync-ce branch June 7, 2021 06:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.StorageSync/storageSyncServices
3 participants