-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updating test scripts #130
Conversation
break | ||
;; | ||
NonHNS-SharedKey-Blob) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation is not proper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
runHNSOAuthDFSIngressBlobTest | ||
runNonHNSOAuthDFSIngressBlobTest | ||
|
||
# runHNSOAuthTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove the comments
fnsBlobConfigFileCheck "$accountName" | ||
PROPERTIES=("fs.azure.account.auth.type") | ||
VALUES=("SharedKey") | ||
triggerRun "NonHNS-SharedKey-Blob" "${accountName}_blob" "$runTest" $processCount "$cleanUpTestContainers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ${accountName}_blob works ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be working...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's creating new file if not already created.
It's showing the same file name in abfs-test-combinations-config.xml for the required combo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on the first PR @manika137, thanks for the patch. Great code. Some thoughts.
triggerRun "AppendBlob-NonHNS-OAuth-Blob" "${accountName}_blob" "$runTest" $processCount "$cleanUpTestContainers" | ||
} | ||
|
||
runHNSOAuthDFSIngressBlobTest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blob ingress on an hns account, dont think it should happen. @anmolanmol1234, what you feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work @saxenapranav. We have added support to handle it
{ | ||
accountName=$(xmlstarlet sel -t -v '//property[name = "fs.azure.hnsTestAccountName"]/value' -n $azureTestXmlPath) | ||
PROPERTIES=("fs.azure.account.auth.type" "fs.azure.ingress.service.type") | ||
VALUES=("OAuth" "blob") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be DFS or BLOB. Not in the uppercase. Reason being, in the code, it resolves from enum AbfsServiceType which has the values in uppercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Anuj has added support for it being case insensitive, right @anujmodi2021 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was supposed to be case insensitive but looks like it's not.
May be that's why ingress related tests are failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this and make it case insensitive, meanwhile @manika137 can you trigger a ingressBlob combination after changing this setting value from blob to BLOB. to confirm this hypothesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Work with the patch.
Added a change request.
@@ -126,39 +177,95 @@ do | |||
esac | |||
done | |||
|
|||
## SECTION: CHECK FOR FNS+BLOB XML FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this section and function to testsupport.sh along with other utility functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, makes sense
…into testScriptUpdate
+1 Looks good. Thanks for taking the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Thanks for the changes!
Description of PR
Updating test scripts.
HNS-OAuth-DFS
HNS-SharedKey-DFS
NonHNS-SharedKey-DFS
AppendBlob-HNS-OAuth-DFS
NonHNS-SharedKey-Blob
NonHNS-OAuth-DFS
NonHNS-OAuth-Blob
AppendBlob-NonHNS-OAuth-Blob
HNS-Oauth-DFS-IngressBlob
NonHNS-Oauth-DFS-IngressBlob