-
Notifications
You must be signed in to change notification settings - Fork 0
[POC] CI automation scripts #133
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
base: wasbDepCodeReview
Are you sure you want to change the base?
Conversation
|
Looks good, can you also share how the results are stored in the storage account ? Like a snapshot of the results stored. |
Added |
anujmodi2021
left a comment
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.
Added a few suggestions, rest looks good.
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/runtests.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/testsupport.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/testsupport.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/testsupport.sh
Outdated
Show resolved
Hide resolved
|
|
||
| runHNSOAuthDFSTest() | ||
| { | ||
| runHNSOAuthDFSTest() { |
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.
these diffs are not needed
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.
Taken
| exit 0 | ||
| ;; | ||
| *) logOutput "ERROR: Invalid selection" | ||
| echo ' ' |
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.
Same dont see any change between new code and old one for this selection. minimizing the diffs would be better for backporting
| azureConfigFilePath="${accountSettingsDir}runresult${accountConfigFileSuffix}" | ||
| testResultsAccountName=$(xmlstarlet sel -t -v '//property[name = "fs.azure.test.results.account.name"]/value' -n $azureConfigFilePath) | ||
| testResultsAccountKey=$(xmlstarlet sel -t -v '//property[name = "fs.azure.test.results.account.key"]/value' -n $azureConfigFilePath) | ||
| branchName="${branchName,,}" |
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.
Are the two commas needed here ?
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.
They are used to convert into lowercase letters in Bash
| if ! command -v az &> /dev/null | ||
| then | ||
| echo "Azure CLI (az) could not be found. Installing Azure CLI..." | ||
| sudo apt update |
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 be under a wait process ? What happens if the installation fails
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. Added a check for it
74a8d40 to
d5e93eb
Compare
d5e93eb to
bfe34e1
Compare
anujmodi2021
left a comment
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.
Added a few more comments.
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/cronjob.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/runtests.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/testsupport.sh
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/dev-support/testrun-scripts/testsupport.sh
Outdated
Show resolved
Hide resolved
397b378 to
00f5e74
Compare
00f5e74 to
548b4f8
Compare
hadoop-tools/hadoop-azure/.gitignore
Outdated
| dev-support/testlogs | ||
| src/test/resources/accountSettings/* | ||
| !src/test/resources/accountSettings/accountName_settings.xml.template | ||
| dev-support/testrun-scripts/cronjob.sh |
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.
EOF Error:
Every file should end with a new line.
anujmodi2021
left a comment
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
| if [[ ! -f "$accountSettingsFile" ]]; | ||
| then | ||
| logOutput "No settings present. Creating new settings file ($accountSettingsFile) from template" | ||
| cp src/test/resources/azure-auth-keys.xml.template $accountSettingsFile |
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.
Path should be taken in variable
| fi | ||
|
|
||
| if [ $runTest == true ] | ||
| if [[ $runTest && $"$IS_CRON_JOB" != "true" ]] |
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.
The variable name format should be camelCase
| AggregatedTestFolder="$testOutputLogFolder" | ||
|
|
||
| checkCronjobDependencies | ||
| az storage container create --name $containerName --account-name $testResultsAccountName --account-key "$testResultsAccountKey" |
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.
what happens if az copy command execution fails ?
| summarycontent | ||
| } >> "$aggregatedTestResult" | ||
| printf "\n :::: AGGREGATED TEST RESULT :::: \n" >> "$aggregatedTestResult" | ||
| printf "\n----- Test results -----\n" |
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.
do we not need to redirect the results now ?
| testResultsAccountName=$(xmlstarlet sel -t -v '//property[name = "fs.azure.test.results.account.name"]/value' -n $azureConfigFilePath) | ||
| testResultsAccountKey=$(xmlstarlet sel -t -v '//property[name = "fs.azure.test.results.account.key"]/value' -n $azureConfigFilePath) | ||
| branchName="${branchName,,}" | ||
| containerName="$(xmlstarlet sel -t -v '//property[name = "fs.azure.container.name"]/value' -n $azureConfigFilePath)" |
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.
Nit: this can also be fs.azure.test.results.container.name?
|
|
||
| uploadToAzure() { | ||
| azureConfigFilePath="${accountSettingsDir}runresult${accountConfigFileSuffix}" | ||
| testResultsAccountName=$(xmlstarlet sel -t -v '//property[name = "fs.azure.test.results.account.name"]/value' -n $azureConfigFilePath) |
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.
What happens if cron job is running and these test result account details are not added?
Description of PR
Automating the daily script runs and saving the results on storage account.
Screenshot of the results stored.
