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.

Sign up for GitHub

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

Update ARM Template Deployment and Fix Transient Test #7352

Merged
merged 1 commit into from
Jan 13, 2020
Merged

Update ARM Template Deployment and Fix Transient Test #7352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...torage/azure-storage-file-share/src/test/java/com/azure/storage/file/share/APISpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,4 +295,12 @@ class APISpec extends Specification {
InputStream getInputStream(byte[] data) {
return new ByteArrayInputStream(data)
}

void sleepIfLive(long milliseconds) {
if (testMode == TestMode.PLAYBACK) {
return
}

sleep(milliseconds)
}
}
6 changes: 5 additions & 1 deletion ...e-storage-file-share/src/test/java/com/azure/storage/file/share/FileSasClientTests.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.azure.storage.file.share

import com.azure.storage.common.StorageSharedKeyCredential

import com.azure.storage.common.implementation.Constants
import com.azure.storage.common.sas.AccountSasPermission
import com.azure.storage.common.sas.AccountSasResourceType
Expand Down Expand Up @@ -116,8 +116,12 @@ class FileSasClientTests extends APISpec {
.setId("0000")
.setAccessPolicy(new ShareAccessPolicy().setPermissions("rcwdl")
.setExpiresOn(getUTCNow().plusDays(1)))

primaryShareClient.setAccessPolicy(Arrays.asList(identifier))

// Sleep 30 seconds if running against the live service as it may take ACLs that long to take effect.
sleepIfLive(30000)
Copy link
Member

Choose a reason for hiding this comment

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

Can we use polling with shorter sleeps instead of sleeping for a fixed 30 seconds? Large, fixed sleeps like this are bad because they make tests slower than necessary and still aren't guaranteed to be reliable.

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 should be do-able with shorter sleeps then poll, mind writing an issue for it as this is a commonly used pattern across the other modules.

Copy link
Member

Choose a reason for hiding this comment

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

Tracking Issue: #7353

I am OK with the sleep in this PR since we are already doing it in other tests. However, I think we should use the same name and code as the existing helper method sleepIfRecord(), instead of adding a new helper method with a different name and implementation.


// Check shareSASPermissions
ShareSasPermission permissions = new ShareSasPermission()
.setReadPermission(true)
Expand Down
16 changes: 16 additions & 0 deletions sdk/storage/test-resources.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,26 @@
"type": "string",
"value": "[listKeys(resourceId('Microsoft.Storage/storageAccounts', variables('premiumAccountName')), variables('storageApiVersion')).keys[0].value]"
},
"AZURE_STORAGE_FILE_ACCOUNT_NAME": {
"type": "string",
"value": "[variables('primaryAccountName')]"
},
"AZURE_STORAGE_FILE_ACCOUNT_KEY": {
"type": "string",
"value": "[listKeys(resourceId('Microsoft.Storage/storageAccounts', variables('primaryAccountName')), variables('storageApiVersion')).keys[0].value]"
},
"AZURE_STORAGE_FILE_ENDPOINT": {
"type": "string",
"value": "[reference(resourceId('Microsoft.Storage/storageAccounts', variables('primaryAccountName')), variables('storageApiVersion')).primaryEndpoints.file]"
},
"AZURE_STORAGE_QUEUE_ACCOUNT_NAME": {
"type": "string",
"value": "[variables('primaryAccountName')]"
},
"AZURE_STORAGE_QUEUE_ACCOUNT_KEY": {
"type": "string",
"value": "[listKeys(resourceId('Microsoft.Storage/storageAccounts', variables('primaryAccountName')), variables('storageApiVersion')).keys[0].value]"
},
"AZURE_STORAGE_QUEUE_ENDPOINT": {
"type": "string",
"value": "[reference(resourceId('Microsoft.Storage/storageAccounts', variables('primaryAccountName')), variables('storageApiVersion')).primaryEndpoints.queue]"
Expand Down
2 changes: 1 addition & 1 deletion sdk/storage/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@ jobs:
STORAGE_DATA_LAKE_ACCOUNT_KEY: $(STORAGE_DATA_LAKE_ACCOUNT_KEY)
AZURE_STORAGE_FILE_ACCOUNT_NAME: $(AZURE_STORAGE_FILE_ACCOUNT_NAME)
AZURE_STORAGE_FILE_ACCOUNT_KEY: $(AZURE_STORAGE_FILE_ACCOUNT_KEY)
AZURE_STORAGE_QUEUE_ACCOUNT_NAME: $(AZURE_STORAGE_FILE_ACCOUNT_NAME)
AZURE_STORAGE_QUEUE_ACCOUNT_NAME: $(AZURE_STORAGE_QUEUE_ACCOUNT_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

From our latest updates to the resource provisioning scripts you do not need to do the variable indirection here. All of the EnvVars of the form AZURE_STORAGE_QUEUE_ACCOUNT_NAME: $(AZURE_STORAGE_QUEUE_ACCOUNT_NAME) can be removed.

You'll only need to keep the variables that are not set by the ARM template (AZURE_TEST_MODE, AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET)

AZURE_STORAGE_QUEUE_ACCOUNT_KEY: $(AZURE_STORAGE_QUEUE_ACCOUNT_KEY)