-
Notifications
You must be signed in to change notification settings - Fork 83
fix(taskfiles): Enable globstar in main taskfile so that **/* works properly in shell commands; Enable pipefail in main taskfile.
#900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis update applies formatting changes to test files by adjusting line breaks and indentation in assertions and string formatting calls, without modifying test logic or behaviour. Additionally, the Changes
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/core/tests/test-ir_encoding_methods.cpp(2 hunks)components/core/tests/test-kql.cpp(2 hunks)components/core/tests/test-string_utils.cpp(1 hunks)components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsx(1 hunks)taskfile.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/IngestPage/Jobs/index.tsxcomponents/core/tests/test-kql.cppcomponents/core/tests/test-ir_encoding_methods.cppcomponents/core/tests/test-string_utils.cpp
🔇 Additional comments (6)
taskfile.yaml (1)
3-3: Enableglobstarfor recursive globs
Adding theshopt: ["globstar"]directive at the top level correctly enables double-star patterns across all tasks, ensuring that**/*globs behave as intended. This change aligns with Taskfile documentation and PR objectives.components/core/tests/test-string_utils.cpp (1)
212-214: Maintain consistent multi-line formatting inGIVEN
The updatedGIVENblock now splits the descriptive string onto a separate indented line, improving readability while preserving test semantics.components/core/tests/test-kql.cpp (2)
283-284: Refactorfmt::vformatinto a single-line assignment
Consolidating thevformatcall enhances readability by reducing vertical space without losing clarity.
305-306: Preserve multi-linefmt::vformatfor escaped namespace
The multi-line formatting in the escaped namespace case retains clear separation of parameters, aiding future maintenance.components/core/tests/test-ir_encoding_methods.cpp (2)
711-712: Align multi-lineREQUIREassertion indentation
Splitting the condition across two lines improves legibility in the protocol validation test. Behaviour remains unchanged.
834-835: Refine multi-line assertion structure
The reformattedREQUIREnow places the comparison operand on its own line, aligning with the surrounding style.
| * @param root0 | ||
| * @param root0.className |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Add explicit JSDoc @param entries for destructured props
The new @param root0 and @param root0.className annotations clarify the input props. For completeness, consider adding a brief description for the @return tag or swapping it with @returns for consistency with JSDoc conventions.
kirkrodrigues
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the PR title, how about:
fix(taskfiles): Enable `globstar` in main taskfile so that `**/*` works properly in shell commands; Enable `pipefail` in main taskfile.
globstar option globally to allow double star globs to work.globstar in main taskfile so that **/* works properly in shell commands; Enable pipefail in main taskfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
taskfile.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
🔇 Additional comments (1)
taskfile.yaml (1)
4-4: Enable recursive globbing withglobstar
This addition ensures that**/*patterns correctly match files in nested directories across all subtasks, aligning with the PR goal of supporting double-star globs.
| @@ -1,5 +1,8 @@ | |||
| version: "3" | |||
|
|
|||
| set: ["u", "pipefail"] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Optional: Consider enabling exit-on-error (e)
The current set: ["u", "pipefail"] will treat unset variables as errors and cause pipelines to fail if any component fails, but individual failing commands in a pipeline may not abort the task immediately. It’s a common best practice to include e so that the shell exits on any error (equivalent to set -euo pipefail).
You can apply this diff if you want stricter fail-fast behaviour:
-set: ["u", "pipefail"]
+set: ["e", "u", "pipefail"]
Description
The current taskfile doesn't have
globstarenabled as mentioned in the official doc here. This triggers a bug thatfind $DIR/**/*wouldn't be resolved properly. We've seen this problem since some files undercomponents/core/tests/are not properly checked/formatted using the current linting workflow.This PR solves this problem by adding
shopt: ["globstar"]at the beginning of the main taskfile. To align with our settings in util taskfiles, we also set up the pipefall options as we did here.Checklist
breaking change.
Validation performed
**/*under the subtasks.Summary by CodeRabbit
Style
Chores