Fix: logs step will work when failure is there #36314
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the conditional logic for commenting on pull requests based on CI test results. The logic for when comments are added has been inverted, affecting how developers are notified about CI outcomes. Additionally, a new script has been introduced to automate the extraction of tags from JavaScript test files, enhancing metadata reporting capabilities. Changes
Possibly related PRs
Suggested labels
Poem
Tip OpenAI O1 model for chat
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
app/client/generate_csv.sh (1)
14-22: The file processing loop is well-structured, but there's room for improvement. 🔍
Consider simplifying the
awkcommand by using a single regular expression to extract the tags. This will make the code more concise and easier to understand.Make the
grepcommand case-insensitive by adding the-iflag. This will ensure that tags with different letter cases are correctly extracted.Here's an example of how you can modify the code:
- tags=$(awk -v RS='[{}]' '/tags:/ {gsub(/[["\]]/, ""); print $0}' "$file" | grep -Eo '@tag\.[A-Za-z0-9_-]+') + tags=$(awk -v RS='[{}]' '/tags:/ {gsub(/[["]]/, ""); print $2}' "$file" | grep -Eio '@tag\.[a-z0-9_-]+')These changes will make the script more robust and maintainable.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (5)
app/client/cypress/scripts/.yarn/cache/ansi-regex-npm-4.1.1-af0a582bb9-b1a6ee44cb.zipis excluded by!**/.yarn/**app/client/cypress/scripts/.yarn/cache/prompt-sync-npm-4.2.0-1ce4e10166-b14dfb7d25.zipis excluded by!**/.yarn/**app/client/cypress/scripts/.yarn/cache/strip-ansi-npm-5.2.0-275214c316-bdb5f76ade.zipis excluded by!**/.yarn/**app/client/cypress/scripts/.yarn/install-state.gzis excluded by!**/.yarn/**app/client/output.csvis excluded by!**/*.csv
Files selected for processing (3)
- .github/workflows/build-client-server-count.yml (4 hunks)
- .github/workflows/ci-test-limited-with-count.yml (1 hunks)
- app/client/generate_csv.sh (1 hunks)
Additional comments not posted (6)
app/client/generate_csv.sh (3)
1-7: Great job setting up the script header and defining the variables! 👍The shebang line correctly specifies the bash interpreter, and the variables for the folder, postfix, and output CSV file are clearly defined. This makes the script more readable and maintainable.
8-9: Excellent work initializing the CSV file with headers! 🌟Using the
echocommand to write the headers to the CSV file is a straightforward and effective approach. The headers are well-defined and cover the necessary information for each test file.
41-48: Great work appending the data to the CSV file and completing the script! 🎉The script correctly appends the extracted data to the CSV file using the
echocommand and increments the row number for each processed file. The completion message at the end of the script provides useful information about the generated CSV file..github/workflows/ci-test-limited-with-count.yml (1)
365-365: Great job optimizing the workflow! 👍The change in the step condition from
if: always()toif: failure()is a smart move. It ensures that the log files are trimmed only when there is a failure in the previous steps, rather than always executing regardless of the outcome.This optimization will help conserve resources and improve the efficiency of the workflow.
.github/workflows/build-client-server-count.yml (2)
454-454: Inversion of logic detected. Please verify the intended behavior.Similar to the previous comment, the condition for adding a comment on the PR with new CI failures has been changed from checking if
env.ci_test_failedis'true'to checking if it is not'true'. This inverts the logic, causing a comment to be added when there are no CI test failures.Please refer to the previous comment for the suggested fix.
468-468: Inversion of logic detected. Please verify the intended behavior.Similar to the previous comment, the condition for adding a comment when the
ci-test-limited-existing-docker-imagejob is successful has been changed from checking ifenv.ci_test_failedis not'true'to checking if it is'true'. This inverts the logic, causing a comment to be added only when there are CI test failures.Please refer to the previous comment for the suggested fix.
| # Remove brackets and split tags into an array | ||
| IFS=',' read -r -a tagArray <<< "$(echo "$tags" | tr '\n' ',' | sed 's/,$//')" | ||
|
|
||
| # Initialize empty tag variables for up to 9 tags | ||
| tags1=""; tags2=""; tags3=""; tags4=""; tags5="" | ||
| tags6=""; tags7=""; tags8=""; tags9="" | ||
|
|
||
| # Assign tags to respective variables | ||
| if [ ${#tagArray[@]} -gt 0 ]; then tags1="${tagArray[0]}"; fi | ||
| if [ ${#tagArray[@]} -gt 1 ]; then tags2="${tagArray[1]}"; fi | ||
| if [ ${#tagArray[@]} -gt 2 ]; then tags3="${tagArray[2]}"; fi | ||
| if [ ${#tagArray[@]} -gt 3 ]; then tags4="${tagArray[3]}"; fi | ||
| if [ ${#tagArray[@]} -gt 4 ]; then tags5="${tagArray[4]}"; fi | ||
| if [ ${#tagArray[@]} -gt 5 ]; then tags6="${tagArray[5]}"; fi | ||
| if [ ${#tagArray[@]} -gt 6 ]; then tags7="${tagArray[6]}"; fi | ||
| if [ ${#tagArray[@]} -gt 7 ]; then tags8="${tagArray[7]}"; fi | ||
| if [ ${#tagArray[@]} -gt 8 ]; then tags9="${tagArray[8]}"; fi |
There was a problem hiding this comment.
The tag assignment logic works, but let's make it more efficient and readable. 📝
-
Replace the multiple
ifstatements with a loop to assign tags to variables dynamically. This will reduce code duplication and make the script more maintainable. -
Use an array to store the tag variables instead of individual variables. This will simplify the initialization and make the code more concise.
Here's an example of how you can refactor the code:
- # Initialize empty tag variables for up to 9 tags
- tags1=""; tags2=""; tags3=""; tags4=""; tags5=""
- tags6=""; tags7=""; tags8=""; tags9=""
-
- # Assign tags to respective variables
- if [ ${#tagArray[@]} -gt 0 ]; then tags1="${tagArray[0]}"; fi
- if [ ${#tagArray[@]} -gt 1 ]; then tags2="${tagArray[1]}"; fi
- if [ ${#tagArray[@]} -gt 2 ]; then tags3="${tagArray[2]}"; fi
- if [ ${#tagArray[@]} -gt 3 ]; then tags4="${tagArray[3]}"; fi
- if [ ${#tagArray[@]} -gt 4 ]; then tags5="${tagArray[4]}"; fi
- if [ ${#tagArray[@]} -gt 5 ]; then tags6="${tagArray[5]}"; fi
- if [ ${#tagArray[@]} -gt 6 ]; then tags7="${tagArray[6]}"; fi
- if [ ${#tagArray[@]} -gt 7 ]; then tags8="${tagArray[7]}"; fi
- if [ ${#tagArray[@]} -gt 8 ]; then tags9="${tagArray[8]}"; fi
+ # Initialize an array to store tag variables
+ tagVars=("" "" "" "" "" "" "" "" "")
+
+ # Assign tags to respective variables using a loop
+ for i in "${!tagArray[@]}"; do
+ if [ $i -lt 9 ]; then
+ tagVars[$i]="${tagArray[$i]}"
+ fi
+ doneThese changes will make the script more efficient and easier to understand.
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.
| # Remove brackets and split tags into an array | |
| IFS=',' read -r -a tagArray <<< "$(echo "$tags" | tr '\n' ',' | sed 's/,$//')" | |
| # Initialize empty tag variables for up to 9 tags | |
| tags1=""; tags2=""; tags3=""; tags4=""; tags5="" | |
| tags6=""; tags7=""; tags8=""; tags9="" | |
| # Assign tags to respective variables | |
| if [ ${#tagArray[@]} -gt 0 ]; then tags1="${tagArray[0]}"; fi | |
| if [ ${#tagArray[@]} -gt 1 ]; then tags2="${tagArray[1]}"; fi | |
| if [ ${#tagArray[@]} -gt 2 ]; then tags3="${tagArray[2]}"; fi | |
| if [ ${#tagArray[@]} -gt 3 ]; then tags4="${tagArray[3]}"; fi | |
| if [ ${#tagArray[@]} -gt 4 ]; then tags5="${tagArray[4]}"; fi | |
| if [ ${#tagArray[@]} -gt 5 ]; then tags6="${tagArray[5]}"; fi | |
| if [ ${#tagArray[@]} -gt 6 ]; then tags7="${tagArray[6]}"; fi | |
| if [ ${#tagArray[@]} -gt 7 ]; then tags8="${tagArray[7]}"; fi | |
| if [ ${#tagArray[@]} -gt 8 ]; then tags9="${tagArray[8]}"; fi | |
| # Remove brackets and split tags into an array | |
| IFS=',' read -r -a tagArray <<< "$(echo "$tags" | tr '\n' ',' | sed 's/,$//')" | |
| # Initialize an array to store tag variables | |
| tagVars=("" "" "" "" "" "" "" "" "") | |
| # Assign tags to respective variables using a loop | |
| for i in "${!tagArray[@]}"; do | |
| if [ $i -lt 9 ]; then | |
| tagVars[$i]="${tagArray[$i]}" | |
| fi | |
| done |
| @@ -292,7 +292,7 @@ jobs: | |||
| fi | |||
|
|
|||
| - name: Add a comment on the PR with new CI failures | |||
| if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0' | |||
| if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0' | |||
There was a problem hiding this comment.
Inversion of logic detected. Please verify the intended behavior.
The condition for adding a comment on the PR with new CI failures has been changed from checking if env.ci_test_failed is 'true' to checking if it is not 'true'. This inverts the logic, causing a comment to be added when there are no CI test failures.
If the intention is to add a comment only when there are new CI failures, please revert the condition as follows:
-if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0'
+if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0'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.
| if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0' | |
| if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0' |
| @@ -306,7 +306,7 @@ | |||
| ``` | |||
|
|
|||
| - name: Add a comment on the PR when ci-test-limited is successful | |||
| if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0' | |||
| if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0' | |||
There was a problem hiding this comment.
Inversion of logic detected. Please verify the intended behavior.
The condition for adding a comment when the ci-test-limited job is successful has been changed from checking if env.ci_test_failed is not 'true' to checking if it is 'true'. This inverts the logic, causing a comment to be added only when there are CI test failures.
If the intention is to add a comment when the ci-test-limited job succeeds, please revert the condition as follows:
-if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0'
+if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0'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.
| if: env.ci_test_failed == 'true' && needs.file-check.outputs.pr != '0' | |
| if: env.ci_test_failed != 'true' && needs.file-check.outputs.pr != '0' |
Description
This step should only work when failure is there.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.ImportExport"
🔍 Cypress test results
Warning
Tests have not run on the HEAD 4a4df8f yet
Fri, 13 Sep 2024 11:05:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores