Fix: Fix for GHSL-2024-277 #36836
Fix: Fix for GHSL-2024-277 #36836
Conversation
WalkthroughThe pull request introduces modifications to two GitHub Actions workflow files: Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci-test-limited-with-count.yml (2)
Line range hint
370-387: Excellent work on improving our test execution process!Class, let's give a round of applause for the improvements in our test execution! The use of
cypress-repeat-proallows us to run our tests multiple times, which is fantastic for catching those tricky, intermittent issues.I particularly like the addition of the
ci_test_status.txtfile. It's like our own report card for the tests!However, we have a small formatting issue to fix. Remember, in our coding class, we always strive for neatness:
- cat cy-repeat-summary.txt + cat cy-repeat-summary.txtLet's remove that extra space at the end of line 372. It's like making sure our desks are tidy at the end of the day!
Keep up the great work, and remember, attention to detail is key in both coding and life!
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 272-272: trailing spaces
(trailing-spaces)
272-272: Let's tidy up our code, shall we?Class, we have a small housekeeping task. On line 272, we have an uninvited guest - a trailing space! Let's show it the door:
- +Remember, keeping our code clean is like keeping our classroom tidy. It makes everything easier to read and understand. Good habits in coding start with these small details!
🧰 Tools
🪛 yamllint
[error] 272-272: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/ci-test-limited-with-count.yml (1 hunks)
- .github/workflows/ci-test-limited.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/ci-test-limited-with-count.yml
270-270: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-limited.yml
260-260: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/ci-test-limited-with-count.yml
[error] 272-272: trailing spaces
(trailing-spaces)
.github/workflows/ci-test-limited.yml
[error] 262-262: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/ci-test-limited-with-count.yml (1)
Line range hint
16-19: Well done on adding the new 'run_count' parameter!Class, let's take a moment to appreciate the addition of the
run_countparameter to both theworkflow_dispatchandworkflow_calltriggers. This enhancement provides us with the flexibility to specify how many times we want to repeat our test runs. It's like giving us the power to practice our math problems as many times as we need!Also applies to: 30-33
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 272-272: trailing spaces
(trailing-spaces)
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | ||
| run: | | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | ||
| COMMIT_MESSAGE="$EVENT_COMMITS" | ||
|
|
||
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | ||
| else | ||
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | ||
| if [[ "$COMMIT_MESSAGE" == "null" ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | ||
| else | ||
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | ||
| COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class, let's examine the changes in our "Set Commit Message" step.
Now, pay attention to how we're handling our commit messages:
-
We've introduced a new environment variable
COMMIT_INFO_AUTHOR. This is good practice for capturing the author's name. -
The logic for setting the commit message has been improved. We're now handling different scenarios more effectively:
- For repository dispatch events with a PR number
- For workflow dispatch events with a PR number
- For other cases, we're extracting the first line of the commit message
However, there are a few areas where we can improve:
- In line 261, we're not quoting our variable. Remember, class, it's always good to quote your variables in bash to prevent word splitting and globbing. Let's fix that:
-COMMIT_MESSAGE="$EVENT_COMMITS"
+COMMIT_MESSAGE="${EVENT_COMMITS}"- In our comparisons, we're using
-nefor numeric comparison. While this works, it's more appropriate to use!=for string comparisons in this context. Let's update lines 263 and 265:
-if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then-elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then-
There's a trailing space on line 262. Let's remove that to keep our code tidy.
-
Lastly, we should quote our variables in the
echostatements to prevent potential issues. For example, on line 264:
-echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${COMMIT_INFO_MESSAGE}" >> $GITHUB_ENVMake these changes, and your code will be much more robust and follow best practices. Any questions, class?
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 actionlint
260-260: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 262-262: trailing spaces
(trailing-spaces)
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | ||
| run: | | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | ||
| COMMIT_MESSAGE="$EVENT_COMMITS" | ||
|
|
||
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | ||
| else | ||
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | ||
| if [[ "$COMMIT_MESSAGE" == "null" ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | ||
| else | ||
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | ||
| COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV |
There was a problem hiding this comment.
Let's improve our commit message handling, shall we?
Good job on updating the commit message handling! You've made it more comprehensive, like adding different chapters to our history book. However, we need to make a few corrections:
- On line 273, use
!=instead of-newhen comparing strings. It's like using the correct comparison operator in our math class! - Remember to use double quotes around our variables to prevent any unexpected word splitting. It's similar to putting quotation marks around a direct quote in our English essays.
Here's how we can fix these issues:
- if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+ if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
- elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+ elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ "$COMMIT_MESSAGE" == "null" ]]; then
- echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
+ echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV"
else
- COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
+ COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^"//')
- echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
+ echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV"
fi
fiThese changes will make our code more robust, just like double-checking our work in class!
📝 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.
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | |
| COMMIT_MESSAGE="$EVENT_COMMITS" | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | |
| if [[ "$COMMIT_MESSAGE" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | |
| else | |
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | |
| COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | |
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| COMMIT_MESSAGE="$EVENT_COMMITS" | |
| if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | |
| elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ "$COMMIT_MESSAGE" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV" | |
| else | |
| COMMIT_FIRST_LINE=$(echo "$COMMIT_MESSAGE" | awk -F '\\\\n' '{print $1}' | sed 's/^"//') | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV" | |
| fi | |
| fi |
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:3:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:5:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:6:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:9:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:12:54: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
[error] 272-272: trailing spaces
(trailing-spaces)
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | ||
| COMMIT_MESSAGE="$EVENT_COMMITS" |
There was a problem hiding this comment.
Is this necessary? Any reason we are assigning one env variable to another?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci-test-limited-with-count.yml (1)
Line range hint
363-379: Excellent improvements to our test execution, class!I'm impressed with the enhancements you've made to our Cypress test execution. Let's review what you've done well:
You've utilized the
run_countinput in thenpx cypress-repeat-procommand, allowing for flexible test repetition. This is like giving us the option to take a practice test multiple times!The introduction of the
ci_test_status.txtfile is a clever way to store our test results. It's like having a clear "pass" or "fail" stamp on our test papers.However, I have a small suggestion to make our result checking even more robust:
- if ! grep -q "Total Failed: 0" cy-repeat-summary.txt; then + if ! grep -q "^Total Failed: 0$" cy-repeat-summary.txt; then echo "ci_test_failed=true" > "$FAILURE_FLAG_FILE" else echo "ci_test_failed=false" > "$FAILURE_FLAG_FILE" fiBy using
^and$in our grep pattern, we ensure we're matching the exact line "Total Failed: 0". This prevents false positives if the number 0 appears elsewhere in the context of failures.Keep up the good work, class! Your attention to detail in improving our testing process is commendable.
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/ci-test-limited-with-count.yml (1 hunks)
- .github/workflows/ci-test-limited.yml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/ci-test-limited-with-count.yml
270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ci-test-limited.yml
260-260: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/ci-test-limited-with-count.yml (2)
Line range hint
15-18: Excellent addition of therun_countparameter, class!I'm pleased to see you've added the
run_countinput parameter to both theworkflow_dispatchandworkflow_calltriggers. This shows great foresight in allowing for flexible test execution. It's like giving us the ability to practice a math problem multiple times to ensure we've truly mastered it!Remember, class, flexibility in our workflows is key to thorough testing and debugging. Well done!
Also applies to: 25-28
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
Line range hint
387-392: Well done on improving our artifact management, class!I'm delighted to see the improvements you've made to our artifact uploads. Let's review what you've done:
You've added a new artifact
ci_test_statuswhich uploads theci_test_status.txtfile. This is like creating a quick summary sheet of our test results - very efficient!You've set the
overwriteoption totruefor all artifact uploads. This ensures we always have the most up-to-date information, just like keeping our class notes current.These changes will make it much easier for us to track and share our test results. It's like having a well-organized binder for all our important documents. Keep up the excellent work!
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | ||
| run: | | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | ||
| else | ||
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | ||
| if [[ "$EVENT_COMMITS" == "null" ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | ||
| else | ||
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | ||
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Class, let's examine the changes in our "Set Commit Message" step.
I'm pleased to see improvements in how we're handling our commit messages. However, there are a few areas where we can make our code even better:
-
We've introduced a new environment variable
COMMIT_INFO_AUTHOR. This is good practice for capturing the author's name. -
The logic for setting the commit message has been improved. We're now handling different scenarios more effectively:
- For repository dispatch events with a PR number
- For workflow dispatch events with a PR number
- For other cases, we're extracting the first line of the commit message
However, there are a few areas where we need to make corrections:
- In our comparisons, we're using
-nefor numeric comparison. While this works, it's more appropriate to use!=for string comparisons in this context. Let's update lines 261 and 263:
-if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then-elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then- We should quote our variables to prevent potential issues with word splitting and globbing. For example, on line 262:
-echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${COMMIT_INFO_MESSAGE}" >> $GITHUB_ENV- Similarly, let's quote the
$EVENT_COMMITSvariable on line 266:
-if [[ "$EVENT_COMMITS" == "null" ]]; then
+if [[ "${EVENT_COMMITS}" == "null" ]]; then- For consistency, let's also quote
$COMMIT_INFO_AUTHORon line 267:
-echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${COMMIT_INFO_AUTHOR}" >> $GITHUB_ENV- Lastly, let's quote
$COMMIT_FIRST_LINEon line 270:
-echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
+echo "COMMIT_INFO_MESSAGE=${COMMIT_FIRST_LINE}" >> $GITHUB_ENVMake these changes, and your code will be much more robust and follow best practices. Any questions, class?
📝 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.
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | |
| if [[ "$EVENT_COMMITS" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | |
| else | |
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | |
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | |
| fi | |
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${COMMIT_INFO_MESSAGE}" >> $GITHUB_ENV | |
| elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ "${EVENT_COMMITS}" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${COMMIT_INFO_AUTHOR}" >> $GITHUB_ENV | |
| else | |
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | |
| echo "COMMIT_INFO_MESSAGE=${COMMIT_FIRST_LINE}" >> $GITHUB_ENV | |
| fi |
🧰 Tools
🪛 actionlint
260-260: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
260-260: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | ||
| run: | | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | ||
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | ||
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | ||
| else | ||
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | ||
| if [[ "$EVENT_COMMITS" == "null" ]]; then | ||
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | ||
| else | ||
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | ||
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | ||
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
Let's improve our commit message handling, shall we?
Class, you've made a commendable effort in updating the commit message handling. It's like you're writing a more detailed entry in our class journal. However, we need to make a few corrections:
-
On lines 271 and 273, we're using
-nefor string comparison. In bash, we should use!=for comparing strings. It's like using the correct comparison operator in our math class! -
We need to put our variables in double quotes to prevent any unexpected word splitting. It's similar to putting quotation marks around a direct quote in our English essays.
Let's fix these issues:
- if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then
+ if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV
- elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then
+ elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then
echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV
else
if [[ "$EVENT_COMMITS" == "null" ]]; then
- echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV
+ echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV"
else
- COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')
+ COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^"//')
- echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV
+ echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV"
fi
fiThese changes will make our code more robust, just like double-checking our work in class!
📝 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.
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ env.COMMIT_INFO_MESSAGE }}" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{github.event_name}} == 'workflow_dispatch' ]]; then | |
| if [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | |
| elif [[ ${{ inputs.pr }} -ne 0 && ${{ github.event_name }} == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ '${{env.EVENT_COMMITS}}' == 'null' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by ${{ env.COMMIT_INFO_AUTHOR }}" >> $GITHUB_ENV | |
| if [[ "$EVENT_COMMITS" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> $GITHUB_ENV | |
| else | |
| echo "COMMIT_INFO_MESSAGE=$(echo \"${{ env.EVENT_COMMITS }}\" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//')" >> $GITHUB_ENV | |
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^\"//') | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> $GITHUB_ENV | |
| fi | |
| COMMIT_INFO_AUTHOR: ${{ github.event.commits[0].author.name }} | |
| run: | | |
| if [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'repository_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_INFO_MESSAGE" >> $GITHUB_ENV | |
| elif [[ "${{ inputs.pr }}" != "0" && "${{ github.event_name }}" == 'workflow_dispatch' ]]; then | |
| echo "COMMIT_INFO_MESSAGE=Workflow run on PR# ${{ inputs.pr }}" >> $GITHUB_ENV | |
| else | |
| if [[ "$EVENT_COMMITS" == "null" ]]; then | |
| echo "COMMIT_INFO_MESSAGE=${{ github.event_name }} by $COMMIT_INFO_AUTHOR" >> "$GITHUB_ENV" | |
| else | |
| COMMIT_FIRST_LINE=$(echo "$EVENT_COMMITS" | awk -F '\\\\n' '{print $1}' | sed 's/^"//') | |
| echo "COMMIT_INFO_MESSAGE=$COMMIT_FIRST_LINE" >> "$GITHUB_ENV" | |
| fi |
🧰 Tools
🪛 actionlint
270-270: shellcheck reported issue in this script: SC2309:warning:1:7: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:2:54: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2309:warning:3:9: -ne treats this as a variable. Use != to compare as string (or expand explicitly with $var)
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:4:70: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:7:83: Double quote to prevent globbing and word splitting
(shellcheck)
270-270: shellcheck reported issue in this script: SC2086:info:10:54: Double quote to prevent globbing and word splitting
(shellcheck)
Description
Fix : https://github.com/appsmithorg/appsmith/security/advisories/GHSA-5f24-4j99-h3gc#event-330907
Use bash syntax to access the environment variable: $EVENT_COMMITS
Fixes #
36835Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 18af836 yet
Mon, 14 Oct 2024 04:55:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
run_countinput parameter for specifying the number of test repetitions.COMMIT_INFO_AUTHORcaptures the author name for better context in commit messages.Improvements