[GLACIER] Fix task ID regex #9203
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughSingle-file change in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TapeCloudUtils
Caller->>TapeCloudUtils: call tapecloud_failure_handler(error, stdout)
alt stdout matches /task ID is .../
TapeCloudUtils->>TapeCloudUtils: match = stdout.match(/task ID is ([\w]+(:[\w]+)*)/)
TapeCloudUtils->>TapeCloudUtils: taskId = match[1]
TapeCloudUtils->>Caller: throw enriched error including taskId
else no match
TapeCloudUtils->>Caller: rethrow original error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
tangledbytes
force-pushed
the
utkarsh/fix/taskid
branch
from
e7905e7 to
467d36c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sdk/glacier_tapecloud.js (1)
195-197: JSDoc whitespace tidy-up looks goodDocblocks read cleaner; no API surface change. Thanks for keeping these consistent.
Also applies to: 265-266, 311-312, 375-376
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/sdk/glacier_tapecloud.js(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-18T05:00:21.149Z
Learnt from: tangledbytes
PR: noobaa/noobaa-core#9183
File: src/sdk/glacier_tapecloud.js:273-311
Timestamp: 2025-08-18T05:00:21.149Z
Learning: In TapeCloudGlacier.migrate method, ignoring the boolean return value from _migrate() and always proceeding to unstage entries is acceptable behavior - the migrate operation doesn't require checking the return value like other operations might.
Applied to files:
src/sdk/glacier_tapecloud.js
📚 Learning: 2025-08-18T05:00:52.294Z
Learnt from: tangledbytes
PR: noobaa/noobaa-core#9183
File: src/sdk/glacier_tapecloud.js:377-406
Timestamp: 2025-08-18T05:00:52.294Z
Learning: In TapeCloudGlacier.restore method, the method always returns true (rather than propagating _recall() result) because it uses a staging approach where the original log file should always be deleted after processing. Failed entries are captured by failure_recorder for retry through staged files, so the original log preservation is not needed.
Applied to files:
src/sdk/glacier_tapecloud.js
📚 Learning: 2025-08-05T10:05:51.959Z
Learnt from: tangledbytes
PR: noobaa/noobaa-core#9183
File: src/sdk/glacier_tapecloud.js:507-509
Timestamp: 2025-08-05T10:05:51.959Z
Learning: In TapeCloudGlacier._finalize_restore method, when failure_recorder is not provided but the DMAPI premig check fails, the code intentionally throws an error to cause the entire batch to fail and preserve the log file for retry, preventing files from being silently stuck in an unrestored state.
Applied to files:
src/sdk/glacier_tapecloud.js
🧬 Code graph analysis (1)
src/sdk/glacier_tapecloud.js (1)
src/endpoint/s3/s3_utils.js (1)
match(152-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
Comment on lines
+123
to
126
| const match = stdout.match(/task ID is ([\w]+(:[\w]+)*)/); | ||
| if (!match || match.length < 2) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing stdout and anchor the regex to a line
Avoid a TypeError when error.stdout is absent, and reduce false positives by matching a whole line (multiline). Also, since we now accept word chars and colons, the nearby comment implying numeric-only segments is misleading—consider updating it.
Apply:
- const match = stdout.match(/task ID is ([\w]+(:[\w]+)*)/);
- if (!match || match.length < 2) {
- throw error;
- }
+ const stdoutStr = typeof stdout === 'string' ? stdout : '';
+ const match = stdoutStr.match(/^\s*task ID is ([\w]+(?:[:][\w]+)*)\s*$/m);
+ if (!match) {
+ throw error;
+ }Optional (if hyphens or dots can appear in task IDs), broaden the charset to [\w.-]+ and add a test. Happy to update if confirmed.
📝 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.
Suggested change
| const match = stdout.match(/task ID is ([\w]+(:[\w]+)*)/); | |
| if (!match || match.length < 2) { | |
| throw error; | |
| } | |
| const stdoutStr = typeof stdout === 'string' ? stdout : ''; | |
| const match = stdoutStr.match(/^\s*task ID is ([\w]+(?:[:][\w]+)*)\s*$/m); | |
| if (!match) { | |
| throw error; | |
| } |
tangledbytes
force-pushed
the
utkarsh/fix/taskid
branch
from
467d36c to
e01186c
Compare
tangledbytes
force-pushed
the
utkarsh/fix/taskid
branch
from
e01186c to
b14a1e3
Compare
naveenpaul1
approved these changes
Sep 15, 2025
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
tangledbytes
force-pushed
the
utkarsh/fix/taskid
branch
from
b14a1e3 to
667baeb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the Problem
Previous regex was pretty rigid in what kind of task id it can accept which is not correct. Task ID can be of the following types:
task ID is tmp15nwx2vl:1000:1002task ID is 1000:1002:1003:1004:1005:1006task ID is 1000task ID is tmp15nwx2vltask ID is tmp15nwx2vl_123:tmpn9km_wyx:1001Explain the Changes
PR changes the regex to
task ID is ([\w]+(:[\w]+)*)(as suggested my Sasaki-san).Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Bug Fixes
Documentation