Update ARM Template Deployment and Fix Transient Test #7352
Conversation
…nt variable name, fixed transient test that was missing a sleep
alzimmermsft
requested review from
danieljurek,
gapra-msft,
jaschrep-msft,
rickle-msft and
sima-zhu
as code owners
|
@hemanttanwar and @danieljurek could you validate the changes made to the ARM deployment template? |
|
/azp run java - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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)
alzimmermsft
dismissed
mikeharder’s stale review
Agreed to work on follow-up issue
ACCOUNT_NAMEandACCOUNT_KEY.SignedIdentifiers, these take up to 30 seconds to take effect and resolves a transient testing issue.