Creating a template for bootstrap, improving fail detection on Linux #22
Conversation
nfbot
added
Type: Bug
WalkthroughThis pull request introduces a new YAML configuration file, Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
bootstrap.yml (3)
1-16: Consider enhancing parameter documentation and defaults.The parameters are well-structured, but could benefit from:
- Adding descriptions to clarify the expected values and usage
- Setting a default value for
AZURE_DEVOPS_PROJECTto improve usabilityparameters: - name: AZURE_DEVOPS_ORG displayName: Azure DevOps organization type: string default: nanoFramework + description: The Azure DevOps organization name - name: AZURE_DEVOPS_PROJECT displayName: Azure DevOps project type: string + default: nanoFramework + description: The Azure DevOps project name - name: AZURE_DEVOPS_PIPELINE_ID displayName: Azure DevOps pipeline ID type: number + description: The ID of the pipeline to trigger - name: AZURE_POOL_NAME displayName: Azure DevOps pool name type: string default: TestStream + description: The name of the agent pool to use
17-17: Fix YAML formatting issues.The YAML file has several formatting issues:
- Trailing spaces on lines 17, 76, 106, and 109
- Missing newline at end of file
-steps: +steps: - repositories: + repositories: - env: + env: - + # Add newline at end of fileAlso applies to: 76-76, 106-106, 109-109, 148-148
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
1-148: Overall assessment of the bootstrap implementation.The implementation successfully achieves the PR objective of improving fail detection for Linux environments through a robust pipeline trigger and monitoring solution. However, several critical areas need attention:
- Security: PAT handling and URL construction need to be more secure
- Error Handling: Add proper error handling for API calls and JSON parsing
- Monitoring: Implement timeout and better error recovery in the monitoring loop
- Documentation: Enhance parameter documentation
These improvements will make the implementation more robust and maintainable.
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
test.yml (3)
Line range hint
1-15: LGTM! Consider documenting the MaxRetries parameter.The parameters section is well-structured, with the new MaxRetries parameter providing configurable retry logic. The detailed documentation for unitTestRunsettings is particularly helpful.
Consider adding similar detailed documentation for the MaxRetries parameter, explaining its purpose and impact on the test execution process.
Line range hint
74-183: Consider enhancing the retry mechanism with exponential backoff.The current implementation uses a fixed delay between retries. While functional, it could be improved by implementing an exponential backoff strategy for more robust error handling.
Consider modifying the delay calculation to use exponential backoff:
- DELAY=2 + # Initial delay of 2 seconds, doubled each retry up to 32 seconds + initial_delay=2 + max_delay=32For Linux:
- sleep $DELAY + current_delay=$((initial_delay * 2 ** (attempt - 1))) + if [ $current_delay -gt $max_delay ]; then + current_delay=$max_delay + fi + sleep $current_delayFor Windows:
- Start-Sleep -Seconds $DELAY + $current_delay = [Math]::Min($initial_delay * [Math]::Pow(2, ($attempt - 1)), $max_delay) + Start-Sleep -Seconds $current_delay
Line range hint
274-321: Add similar failure detection for Windows test execution.The Linux implementation includes enhanced failure detection that's missing in the Windows version. This creates an asymmetry in how test failures are handled across platforms.
Add the following condition to the Windows test execution:
} elseif ($RESULT_TEST -match "The argument .* is invalid.") { Write-Output "Test failed as arguments are not valid." exit 1 + } elseif ($RESULT_TEST[-5..-1] -match "Failed: [1-9][0-9]*") { + Write-Output "Test failed with non-zero failures." + exit 1 } else { break }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
azure-boostrap.yamlis excluded by none and included by none
📒 Files selected for processing (2)
bootstrap.yml(1 hunks)test.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
bootstrap.yml
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
test.yml (1)
210-212: LGTM! Enhanced failure detection for Linux tests.
The new condition effectively captures test failures by checking for non-zero failure counts in the test output.
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | ||
| AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url") |
There was a problem hiding this comment.
Protect against URL injection vulnerabilities.
The script constructs URLs using environment variables without proper URL encoding.
+ # URL encode function
+ urlencode() {
+ local string="${1}"
+ local strlen=${#string}
+ local encoded=""
+ local pos c o
+ for (( pos=0 ; pos<strlen ; pos++ )); do
+ c=${string:$pos:1}
+ case "$c" in
+ [-_.~a-zA-Z0-9] ) o="${c}" ;;
+ * ) printf -v o '%%%02x' "'$c"
+ esac
+ encoded+="${o}"
+ done
+ echo "${encoded}"
+ }
+
- url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1"
+ encoded_pool_name=$(urlencode "${poolName}")
+ url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${encoded_pool_name}&api-version=7.1"Also applies to: 51-52, 89-90
| branch="${BUILD_SOURCEBRANCH}" | ||
|
|
||
| # Encode the PAT | ||
| patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64) |
There was a problem hiding this comment.
Consider using a more secure method for PAT handling.
The current implementation echoes the PAT in plain text which could expose it in logs.
- patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64)
+ patEncoded=$(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64) | |
| patEncoded=$(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64) |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | ||
| AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url") | ||
| poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id') | ||
|
|
There was a problem hiding this comment.
Add error handling for jq operations and curl timeout.
The script doesn't handle potential failures in JSON parsing or API timeouts.
- url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1"
- AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url")
- poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id')
+ url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1"
+ AZP_POOL_AGENTS=$(curl -s --max-time 30 "${headers[@]}" -X GET "$url")
+ if ! poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id'); then
+ echo "Failed to parse pool ID from response"
+ exit 1
+ fi
+ if [ "$poolId" == "null" ]; then
+ echo "Pool not found: ${poolName}"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | |
| AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url") | |
| poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id') | |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | |
| AZP_POOL_AGENTS=$(curl -s --max-time 30 "${headers[@]}" -X GET "$url") | |
| if ! poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id'); then | |
| echo "Failed to parse pool ID from response" | |
| exit 1 | |
| fi | |
| if [ "$poolId" == "null" ]; then | |
| echo "Pool not found: ${poolName}" | |
| exit 1 | |
| fi |
| while true; do | ||
| response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | ||
| http_code=${response: -3} | ||
| content=${response::-3} | ||
|
|
||
| if [ $http_code -eq 200 ]; then | ||
| state=$(echo "$content" | jq -r '.state') | ||
| result=$(echo "$content" | jq -r '.result') | ||
|
|
||
| echo "Pipeline run state: $state" | ||
|
|
||
| if [ "$state" == "completed" ]; then | ||
| echo "Pipeline run completed with result: $result" | ||
| if [ "$result" == "succeeded" ]; then | ||
| exit 0 | ||
| else | ||
| exit 1 | ||
| fi | ||
| fi | ||
| else | ||
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | ||
| echo "Response: $content" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for a while before checking again | ||
| sleep 30 | ||
| done |
There was a problem hiding this comment.
Add timeout and improve error handling in monitoring loop.
The monitoring loop lacks a maximum duration and proper error handling for network issues.
+ # Maximum monitoring duration (2 hours)
+ end_time=$((SECONDS + 7200))
+ # Initialize consecutive failure counter
+ failures=0
+
while true; do
+ # Check if maximum duration exceeded
+ if [ $SECONDS -gt $end_time ]; then
+ echo "Monitoring timed out after 2 hours"
+ exit 1
+ fi
+
- response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url")
+ response=$(curl -s -w "%{http_code}" --max-time 30 -H "Authorization: Basic $(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url")
http_code=${response: -3}
content=${response::-3}
if [ $http_code -eq 200 ]; then
+ failures=0
state=$(echo "$content" | jq -r '.state')
result=$(echo "$content" | jq -r '.result')
- echo "Pipeline run state: $state"
+ # Only log state changes
+ if [ "$state" != "$last_state" ]; then
+ echo "Pipeline run state: $state"
+ last_state="$state"
+ fi
if [ "$state" == "completed" ]; then
echo "Pipeline run completed with result: $result"
if [ "$result" == "succeeded" ]; then
exit 0
else
exit 1
fi
fi
else
+ ((failures++))
+ if [ $failures -ge 3 ]; then
echo "Failed to get pipeline run status. HTTP Status Code: $http_code"
echo "Response: $content"
exit 1
+ fi
fi
# Wait for a while before checking again
sleep 30
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while true; do | |
| response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| state=$(echo "$content" | jq -r '.state') | |
| result=$(echo "$content" | jq -r '.result') | |
| echo "Pipeline run state: $state" | |
| if [ "$state" == "completed" ]; then | |
| echo "Pipeline run completed with result: $result" | |
| if [ "$result" == "succeeded" ]; then | |
| exit 0 | |
| else | |
| exit 1 | |
| fi | |
| fi | |
| else | |
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | |
| echo "Response: $content" | |
| exit 1 | |
| fi | |
| # Wait for a while before checking again | |
| sleep 30 | |
| done | |
| # Maximum monitoring duration (2 hours) | |
| end_time=$((SECONDS + 7200)) | |
| # Initialize consecutive failure counter | |
| failures=0 | |
| while true; do | |
| # Check if maximum duration exceeded | |
| if [ $SECONDS -gt $end_time ]; then | |
| echo "Monitoring timed out after 2 hours" | |
| exit 1 | |
| fi | |
| response=$(curl -s -w "%{http_code}" --max-time 30 -H "Authorization: Basic $(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| failures=0 | |
| state=$(echo "$content" | jq -r '.state') | |
| result=$(echo "$content" | jq -r '.result') | |
| # Only log state changes | |
| if [ "$state" != "$last_state" ]; then | |
| echo "Pipeline run state: $state" | |
| last_state="$state" | |
| fi | |
| if [ "$state" == "completed" ]; then | |
| echo "Pipeline run completed with result: $result" | |
| if [ "$result" == "succeeded" ]; then | |
| exit 0 | |
| else | |
| exit 1 | |
| fi | |
| fi | |
| else | |
| ((failures++)) | |
| if [ $failures -ge 3 ]; then | |
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | |
| echo "Response: $content" | |
| exit 1 | |
| fi | |
| fi | |
| # Wait for a while before checking again | |
| sleep 30 | |
| done |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
bootstrap.yml (3)
1-16: Consider removing hardcoded default values for organization.While the parameters are well-structured, having the organization name hardcoded as a default value might not be the best practice. Consider removing the default value for
AZURE_DEVOPS_ORGto ensure explicit specification during pipeline execution.- name: AZURE_DEVOPS_ORG displayName: Azure DevOps organization type: string - default: nanoFramework
17-17: Fix YAML formatting issues.There are several formatting issues in the file:
- Trailing spaces on lines 17, 76, 106, and 109
- Missing newline at end of file
Please ensure to:
- Remove trailing spaces
- Add a newline at the end of the file
Also applies to: 76-76, 106-106, 109-109, 148-148
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
1-148: Consider extracting reusable components.The script contains several utility functions that could be beneficial across different pipelines:
- Azure DevOps API authentication
- Pipeline status monitoring
- Error handling and retries
Consider extracting these into separate, reusable YAML templates to promote code reuse and maintainability.
Would you like assistance in creating these reusable templates?
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
test.yml (2)
Line range hint
73-142: Consider extracting baud rates to pipeline parameters.The flash device scripts for both Linux and Windows contain hardcoded baud rates. Consider moving these to pipeline parameters for better maintainability and flexibility.
Example modification:
parameters: - name: baudRates type: object default: [1500000, 1000000, 500000, 250000, 150000] displayName: "Baud rates to try for flashing (in order)"Then use
${{ parameters.baudRates }}in the scripts.
Line range hint
312-349: Add condition to cleanup scripts.The cleanup scripts might remove .trx files before they're successfully published if the PublishTestResults task fails. Consider adding a condition to only run cleanup when publishing succeeds.
Modify the cleanup tasks:
displayName: Linux Cleaning tests results + condition: and(eq(variables['Agent.OS'], 'Linux'), succeeded('PublishTestResults@2')) - condition: eq(variables['Agent.OS'], 'Linux') displayName: 'Windows Cleaning tests results' + condition: and(eq(variables['Agent.OS'], 'Windows_NT'), succeeded('PublishTestResults@2')) - condition: eq(variables['Agent.OS'], 'Windows_NT')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
azure-boostrap.yamlis excluded by none and included by none
📒 Files selected for processing (2)
bootstrap.yml(1 hunks)test.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
bootstrap.yml
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
test.yml (1)
Line range hint 1-15: LGTM! Well-documented parameters.
The parameters are well-structured with clear documentation:
- The new
MaxRetriesparameter provides consistent retry behavior across the pipeline - The enhanced documentation for
unitTestRunsettingsclearly explains the expected format and provides helpful examples
bootstrap.yml
Outdated
| - script: | | ||
| # Validate required environment variables | ||
| for var in AZURE_DEVOPS_ORG AZURE_DEVOPS_PROJECT AZURE_DEVOPS_PIPELINE_ID AZURE_POOL_NAME; do | ||
| if [ -z "${!var}" ]; then | ||
| echo "Error: Required environment variable $var is not set" | ||
| exit 1 | ||
| fi | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for AZURE_DEVOPS_PAT environment variable.
The script validates several environment variables but misses the critical AZURE_DEVOPS_PAT variable. This could lead to unclear errors later in the script.
# Validate required environment variables
- for var in AZURE_DEVOPS_ORG AZURE_DEVOPS_PROJECT AZURE_DEVOPS_PIPELINE_ID AZURE_POOL_NAME; do
+ for var in AZURE_DEVOPS_ORG AZURE_DEVOPS_PROJECT AZURE_DEVOPS_PIPELINE_ID AZURE_POOL_NAME AZURE_DEVOPS_PAT; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - script: | | |
| # Validate required environment variables | |
| for var in AZURE_DEVOPS_ORG AZURE_DEVOPS_PROJECT AZURE_DEVOPS_PIPELINE_ID AZURE_POOL_NAME; do | |
| if [ -z "${!var}" ]; then | |
| echo "Error: Required environment variable $var is not set" | |
| exit 1 | |
| fi | |
| done | |
| - script: | | |
| # Validate required environment variables | |
| for var in AZURE_DEVOPS_ORG AZURE_DEVOPS_PROJECT AZURE_DEVOPS_PIPELINE_ID AZURE_POOL_NAME AZURE_DEVOPS_PAT; do | |
| if [ -z "${!var}" ]; then | |
| echo "Error: Required environment variable $var is not set" | |
| exit 1 | |
| fi | |
| done |
| # Encode the PAT | ||
| patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64) | ||
|
|
||
| # Define the headers | ||
| headers=( | ||
| -H "Authorization: Basic $patEncoded" | ||
| -H "Content-Type: application/json" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve PAT encoding security.
The current PAT encoding method might expose the token in process listings. Consider using a more secure approach.
- patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64)
+ patEncoded=$(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64 | tr -d '\n')
# Define the headers
headers=(
- -H "Authorization: Basic $patEncoded"
+ -H "Authorization: Basic ${patEncoded}"
-H "Content-Type: application/json"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Encode the PAT | |
| patEncoded=$(echo -n ":${AZURE_DEVOPS_PAT}" | base64) | |
| # Define the headers | |
| headers=( | |
| -H "Authorization: Basic $patEncoded" | |
| -H "Content-Type: application/json" | |
| ) | |
| # Encode the PAT | |
| patEncoded=$(printf ":%s" "${AZURE_DEVOPS_PAT}" | base64 | tr -d '\n') | |
| # Define the headers | |
| headers=( | |
| -H "Authorization: Basic ${patEncoded}" | |
| -H "Content-Type: application/json" | |
| ) |
| # Get the pool ID | ||
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | ||
| AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url") | ||
| poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id') | ||
|
|
||
| echo "Pool ID: $poolId" | ||
|
|
||
| # Define the URL to get all agents in the pool | ||
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools/${poolId}/agents?includeCapabilities=true&api-version=7.1" | ||
|
|
||
| response=$(curl -s -w "%{http_code}" "${headers[@]}" -X GET "$url") | ||
| http_code=${response: -3} | ||
| content=${response::-3} | ||
|
|
||
| if [ $http_code -eq 200 ]; then | ||
| # Extract all userCapabilities names for online and enabled agents as a unique list | ||
| capabilityNames=$(echo "$content" | jq -r '[.value[] | select(.status == "online" and .enabled == true) | .userCapabilities | keys] | unique | flatten | join("\n- ")') | ||
| else | ||
| echo "Failed to retrieve agent capabilities. HTTP Status Code: $http_code" | ||
| echo "Response: \"$content\"" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and improve error handling for API calls.
The curl commands lack timeout settings and could hang indefinitely. Also, error messages could be more descriptive.
# Get the pool ID
url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1"
- AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url")
+ AZP_POOL_AGENTS=$(curl -s --connect-timeout 10 --max-time 30 "${headers[@]}" -X GET "$url")
poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id')
+ if [ -z "$poolId" ] || [ "$poolId" = "null" ]; then
+ echo "Error: Failed to retrieve pool ID for pool name: ${poolName}"
+ exit 1
+ fi
# Define the URL to get all agents in the pool
url="https://dev.azure.com/${organization}/_apis/distributedtask/pools/${poolId}/agents?includeCapabilities=true&api-version=7.1"
- response=$(curl -s -w "%{http_code}" "${headers[@]}" -X GET "$url")
+ response=$(curl -s --connect-timeout 10 --max-time 30 -w "%{http_code}" "${headers[@]}" -X GET "$url")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get the pool ID | |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | |
| AZP_POOL_AGENTS=$(curl -s "${headers[@]}" -X GET "$url") | |
| poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id') | |
| echo "Pool ID: $poolId" | |
| # Define the URL to get all agents in the pool | |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools/${poolId}/agents?includeCapabilities=true&api-version=7.1" | |
| response=$(curl -s -w "%{http_code}" "${headers[@]}" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| # Extract all userCapabilities names for online and enabled agents as a unique list | |
| capabilityNames=$(echo "$content" | jq -r '[.value[] | select(.status == "online" and .enabled == true) | .userCapabilities | keys] | unique | flatten | join("\n- ")') | |
| else | |
| echo "Failed to retrieve agent capabilities. HTTP Status Code: $http_code" | |
| echo "Response: \"$content\"" | |
| exit 1 | |
| fi | |
| # Get the pool ID | |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools?poolName=${poolName}&api-version=7.1" | |
| AZP_POOL_AGENTS=$(curl -s --connect-timeout 10 --max-time 30 "${headers[@]}" -X GET "$url") | |
| poolId=$(echo "$AZP_POOL_AGENTS" | jq -r '.value[0].id') | |
| if [ -z "$poolId" ] || [ "$poolId" = "null" ]; then | |
| echo "Error: Failed to retrieve pool ID for pool name: ${poolName}" | |
| exit 1 | |
| fi | |
| echo "Pool ID: $poolId" | |
| # Define the URL to get all agents in the pool | |
| url="https://dev.azure.com/${organization}/_apis/distributedtask/pools/${poolId}/agents?includeCapabilities=true&api-version=7.1" | |
| response=$(curl -s --connect-timeout 10 --max-time 30 -w "%{http_code}" "${headers[@]}" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| # Extract all userCapabilities names for online and enabled agents as a unique list | |
| capabilityNames=$(echo "$content" | jq -r '[.value[] | select(.status == "online" and .enabled == true) | .userCapabilities | keys] | unique | flatten | join("\n- ")') | |
| if [ -z "$capabilityNames" ]; then | |
| echo "Warning: No capabilities found for online and enabled agents" | |
| fi | |
| else | |
| echo "Error: Failed to retrieve agent capabilities for pool ID: ${poolId}" | |
| echo "HTTP Status Code: $http_code" | |
| echo "Response: \"$content\"" | |
| exit 1 | |
| fi |
bootstrap.yml
Outdated
| - script: | | ||
| echo "Pipeline to monitor Run ID: $(run_id)" | ||
| echo "Pipeline direct link: https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)" | ||
| # Define the URL to get the pipeline run status | ||
| url="https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_apis/pipelines/${AZURE_DEVOPS_PIPELINE_ID}/runs/$(run_id)?api-version=7.1" | ||
|
|
||
| # Loop to monitor the pipeline run status | ||
| while true; do | ||
| response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | ||
| http_code=${response: -3} | ||
| content=${response::-3} | ||
|
|
||
| if [ $http_code -eq 200 ]; then | ||
| state=$(echo "$content" | jq -r '.state') | ||
| result=$(echo "$content" | jq -r '.result') | ||
|
|
||
| echo "Pipeline run state: $state" | ||
|
|
||
| if [ "$state" == "completed" ]; then | ||
| echo "Pipeline run completed with result: $result" | ||
| if [ "$result" == "succeeded" ]; then | ||
| exit 0 | ||
| else | ||
| exit 1 | ||
| fi | ||
| fi | ||
| else | ||
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | ||
| echo "Response: $content" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for a while before checking again | ||
| sleep 30 | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and improve monitoring resilience.
The monitoring script has several potential issues:
- No maximum duration for monitoring
- No handling of temporary network issues
- Fixed polling interval
+ MAX_DURATION=7200 # 2 hours in seconds
+ START_TIME=$(date +%s)
+ RETRY_COUNT=0
+ MAX_RETRIES=3
+ INITIAL_SLEEP=30
while true; do
+ CURRENT_TIME=$(date +%s)
+ ELAPSED_TIME=$((CURRENT_TIME - START_TIME))
+
+ if [ $ELAPSED_TIME -gt $MAX_DURATION ]; then
+ echo "Error: Pipeline monitoring timed out after $((MAX_DURATION/60)) minutes"
+ exit 1
+ fi
response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url")
http_code=${response: -3}
content=${response::-3}
if [ $http_code -eq 200 ]; then
+ RETRY_COUNT=0
state=$(echo "$content" | jq -r '.state')
result=$(echo "$content" | jq -r '.result')
echo "Pipeline run state: $state"
if [ "$state" == "completed" ]; then
echo "Pipeline run completed with result: $result"
if [ "$result" == "succeeded" ]; then
exit 0
else
exit 1
fi
fi
else
+ RETRY_COUNT=$((RETRY_COUNT + 1))
+ if [ $RETRY_COUNT -gt $MAX_RETRIES ]; then
echo "Failed to get pipeline run status. HTTP Status Code: $http_code"
echo "Response: $content"
exit 1
+ else
+ echo "Temporary error (attempt $RETRY_COUNT/$MAX_RETRIES). Retrying..."
+ fi
fi
- sleep 30
+ SLEEP_TIME=$((INITIAL_SLEEP * (2 ** (RETRY_COUNT - 1))))
+ [ $SLEEP_TIME -gt 300 ] && SLEEP_TIME=300 # Cap at 5 minutes
+ sleep $SLEEP_TIME
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - script: | | |
| echo "Pipeline to monitor Run ID: $(run_id)" | |
| echo "Pipeline direct link: https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)" | |
| # Define the URL to get the pipeline run status | |
| url="https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_apis/pipelines/${AZURE_DEVOPS_PIPELINE_ID}/runs/$(run_id)?api-version=7.1" | |
| # Loop to monitor the pipeline run status | |
| while true; do | |
| response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| state=$(echo "$content" | jq -r '.state') | |
| result=$(echo "$content" | jq -r '.result') | |
| echo "Pipeline run state: $state" | |
| if [ "$state" == "completed" ]; then | |
| echo "Pipeline run completed with result: $result" | |
| if [ "$result" == "succeeded" ]; then | |
| exit 0 | |
| else | |
| exit 1 | |
| fi | |
| fi | |
| else | |
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | |
| echo "Response: $content" | |
| exit 1 | |
| fi | |
| # Wait for a while before checking again | |
| sleep 30 | |
| done | |
| - script: | | |
| echo "Pipeline to monitor Run ID: $(run_id)" | |
| echo "Pipeline direct link: https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)" | |
| # Define the URL to get the pipeline run status | |
| url="https://dev.azure.com/${AZURE_DEVOPS_ORG}/${AZURE_DEVOPS_PROJECT}/_apis/pipelines/${AZURE_DEVOPS_PIPELINE_ID}/runs/$(run_id)?api-version=7.1" | |
| MAX_DURATION=7200 # 2 hours in seconds | |
| START_TIME=$(date +%s) | |
| RETRY_COUNT=0 | |
| MAX_RETRIES=3 | |
| INITIAL_SLEEP=30 | |
| # Loop to monitor the pipeline run status | |
| while true; do | |
| CURRENT_TIME=$(date +%s) | |
| ELAPSED_TIME=$((CURRENT_TIME - START_TIME)) | |
| if [ $ELAPSED_TIME -gt $MAX_DURATION ]; then | |
| echo "Error: Pipeline monitoring timed out after $((MAX_DURATION/60)) minutes" | |
| exit 1 | |
| fi | |
| response=$(curl -s -w "%{http_code}" -H "Authorization: Basic $(echo -n ":${AZURE_DEVOPS_PAT}" | base64)" -X GET "$url") | |
| http_code=${response: -3} | |
| content=${response::-3} | |
| if [ $http_code -eq 200 ]; then | |
| RETRY_COUNT=0 | |
| state=$(echo "$content" | jq -r '.state') | |
| result=$(echo "$content" | jq -r '.result') | |
| echo "Pipeline run state: $state" | |
| if [ "$state" == "completed" ]; then | |
| echo "Pipeline run completed with result: $result" | |
| if [ "$result" == "succeeded" ]; then | |
| exit 0 | |
| else | |
| exit 1 | |
| fi | |
| fi | |
| else | |
| RETRY_COUNT=$((RETRY_COUNT + 1)) | |
| if [ $RETRY_COUNT -gt $MAX_RETRIES ]; then | |
| echo "Failed to get pipeline run status. HTTP Status Code: $http_code" | |
| echo "Response: $content" | |
| exit 1 | |
| else | |
| echo "Temporary error (attempt $RETRY_COUNT/$MAX_RETRIES). Retrying..." | |
| fi | |
| fi | |
| SLEEP_TIME=$((INITIAL_SLEEP * (2 ** (RETRY_COUNT - 1)))) | |
| [ $SLEEP_TIME -gt 300 ] && SLEEP_TIME=300 # Cap at 5 minutes | |
| sleep $SLEEP_TIME | |
| done |
| elif tail -n 5 <<< "$RESULT_TEST" | grep -qE "Failed: [1-9][0-9]*"; then | ||
| echo "Test failed with non-zero failures." | ||
| exit 1 |
There was a problem hiding this comment.
Add failure detection to Windows test execution.
The Linux script includes failure detection by checking the last 5 lines for non-zero failures, but this check is missing in the Windows script. This inconsistency could lead to tests being marked as successful on Windows when they actually failed.
Add equivalent failure detection to the Windows script:
} elseif ($RESULT_TEST -match "The argument .* is invalid.") {
Write-Output "Test failed as arguments are not valid."
exit 1
+ } elseif (($RESULT_TEST -split "`n" | Select-Object -Last 5) -match "Failed: [1-9][0-9]*") {
+ Write-Output "Test failed with non-zero failures."
+ exit 1
} else {
break
}Also applies to: 268-269
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
bootstrap.yml (4)
1-16: Add parameter value validation.Consider adding validation patterns for the parameters to ensure they match expected formats:
- AZURE_DEVOPS_ORG should not contain special characters
- AZURE_DEVOPS_PIPELINE_ID should be positive
- AZURE_POOL_NAME should follow Azure naming conventions
- name: AZURE_DEVOPS_ORG displayName: Azure DevOps organization type: string default: nanoFramework + pattern: '^[a-zA-Z0-9-]+$' - name: AZURE_DEVOPS_PROJECT displayName: Azure DevOps project type: string + pattern: '^[a-zA-Z0-9-_]+$' - name: AZURE_DEVOPS_PIPELINE_ID displayName: Azure DevOps pipeline ID type: number + minimum: 1 - name: AZURE_POOL_NAME displayName: Azure DevOps pool name type: string default: TestStream + pattern: '^[a-zA-Z0-9-_]+$'
68-71: Improve JSON parameter handling and error checking.The jq command for parameter preparation lacks error handling and the output format could be improved for better readability.
- parametersJson=$(jq -n --arg appComponents "- $capabilityNames" '{templateParameters: {appComponents: $appComponents}}') + if ! parametersJson=$(jq -n --arg appComponents "$capabilityNames" \ + '{templateParameters: {appComponents: ($appComponents | split("\n"))}}'); then + echo "Error: Failed to prepare parameters JSON" + exit 1 + fi
112-112: Use environment variables for consistent URL construction.The pipeline link URL is constructed differently from other API URLs and could be inconsistent.
- echo "Pipeline direct link: https://dev.azure.com/${parameters.AZURE_DEVOPS_ORG}/${parameters.AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)" + # Use the same organization URL base as other API calls + base_url="https://dev.azure.com/${parameters.AZURE_DEVOPS_ORG}" + echo "Pipeline direct link: ${base_url}/${parameters.AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)"
17-148: Fix YAML formatting issues.There are several trailing spaces and a missing newline at the end of the file.
- Remove trailing spaces on lines 17, 76, 106, and 109
- Add a newline at the end of the file
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
azure-boostrap.yamlis excluded by none and included by none
📒 Files selected for processing (1)
bootstrap.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
bootstrap.yml
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 76-76: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 109-109: trailing spaces
(trailing-spaces)
[error] 148-148: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
bootstrap.yml (3)
1-16: Add validation for required parameters.The parameters
AZURE_DEVOPS_PROJECTandAZURE_DEVOPS_PIPELINE_IDdon't have default values but also lack required field validation.Add the
required: truefield to these parameters:- name: AZURE_DEVOPS_PROJECT displayName: Azure DevOps project type: string + required: true - name: AZURE_DEVOPS_PIPELINE_ID displayName: Azure DevOps pipeline ID type: number + required: true
103-104: Add logging timestamp to pipeline monitoring.The monitoring output should include timestamps for better debugging.
Add timestamp to log messages:
+ timestamp() { + date '+%Y-%m-%d %H:%M:%S' + } - echo "Pipeline to monitor Run ID: $(run_id)" - echo "Pipeline direct link: https://dev.azure.com/${parameters.AZURE_DEVOPS_ORG}/${parameters.AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)" + echo "$(timestamp) Pipeline to monitor Run ID: $(run_id)" + echo "$(timestamp) Pipeline direct link: https://dev.azure.com/${parameters.AZURE_DEVOPS_ORG}/${parameters.AZURE_DEVOPS_PROJECT}/_build/results?buildId=$(run_id)"
17-17: Fix YAML formatting issues.The YAML file has several formatting issues:
- Trailing spaces on lines 17, 68, 98, and 101
- Missing newline at end of file
Apply these fixes:
- Remove trailing spaces
- Add newline at end of file
Also applies to: 68-68, 98-98, 101-101, 140-140
🧰 Tools
🪛 yamllint
[error] 17-17: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bootstrap.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
bootstrap.yml
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 68-68: trailing spaces
(trailing-spaces)
[error] 98-98: trailing spaces
(trailing-spaces)
[error] 101-101: trailing spaces
(trailing-spaces)
[error] 140-140: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
bootstrap.yml (1)
118-120: Improve URL construction readability.The pipeline URL construction could be made more maintainable by using variables.
- echo "Pipeline direct link: https://dev.azure.com/${{ parameters.AZURE_DEVOPS_ORG }}/${{ parameters.AZURE_DEVOPS_PROJECT }}/_build/results?buildId=$(run_id)" - # Define the URL to get the pipeline run status - url="https://dev.azure.com/${{ parameters.AZURE_DEVOPS_ORG }}/${{ parameters.AZURE_DEVOPS_PROJECT }}/_apis/pipelines/${{ parameters.AZURE_DEVOPS_PIPELINE_ID }}/runs/$(run_id)?api-version=7.1" + # Define base URL components + base_url="https://dev.azure.com/${{ parameters.AZURE_DEVOPS_ORG }}/${{ parameters.AZURE_DEVOPS_PROJECT }}" + + # Construct URLs + echo "Pipeline direct link: ${base_url}/_build/results?buildId=$(run_id)" + url="${base_url}/_apis/pipelines/${{ parameters.AZURE_DEVOPS_PIPELINE_ID }}/runs/$(run_id)?api-version=7.1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
maintainer.mdis excluded by!**/*.mdand included by none
📒 Files selected for processing (1)
bootstrap.yml(1 hunks)
🔇 Additional comments (1)
bootstrap.yml (1)
1-22: LGTM! Well-structured parameter definitions.
The parameters are well-defined with appropriate:
- Validation patterns for security
- Required flags where necessary
- Meaningful default values
- Type constraints
Description
Creating a template for bootstrap, improving fail detection on Linux
Motivation and Context
How Has This Been Tested?
This PR is the test!
Screenshots
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements