DanielCarter-stack commented on PR #10483:
URL: https://github.com/apache/seatunnel/pull/10483#issuecomment-3883507824
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10483", "part": 1,
"total": 1} -->
### Issue 1: Same issue exists in related scripts unrepaired
**Location**:
- `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-cluster.sh:55`
- `seatunnel-core/seatunnel-starter/src/main/bin/seatunnel-connector.sh:45`
**Problem Description**:
`seatunnel-cluster.sh` and `seatunnel-connector.sh` also use the same
`args=$@` pattern and have the same word splitting issue. Although these two
scripts may not frequently use arguments with spaces, for consistency and to
avoid future bugs, they should be fixed together.
**Potential Risks**:
- Risk 1: Users will encounter the same issue if they use configuration file
paths with spaces or other arguments
- Risk 2: Code reviewers may consider this an inconsistent implementation
and may mistakenly think it is intentional
**Impact Scope**:
- Direct impact: Users using `seatunnel-cluster.sh` and
`seatunnel-connector.sh`
- Affected area: Single module (seatunnel-starter)
**Severity**: MINOR
**Improvement Suggestions**:
```bash
# seatunnel-cluster.sh lines 51-56 should be changed to:
if [ $# == 0 ]; then
set -- -h # 或者保持为空 ""
fi
args=("$@")
# Then use "${args[@]}" on lines 198 and 200
# seatunnel-connector.sh lines 41-46 should be changed to:
if [ $# == 0 ]; then
set -- -h
fi
args=("$@")
# Then use "${args[@]}" on line 51
```
**Reasons**:
- Maintain code consistency and maintainability
- Avoid the same bug in the future
- Low fix cost, only requires the same pattern replacement
---
### Issue 2: Lack of automated testing to verify the fix
**Location**:
- Missing file:
`seatunnel-core/seatunnel-starter/src/test/java/org/apache/seatunnel/core/starter/seatunnel/SeaTunnelClientArgParsingIT.java`
(recommended to create)
**Related Context**:
- Existing test:
`seatunnel-e2e/seatunnel-engine-e2e/connector-seatunnel-e2e-base/src/test/java/org/apache/seatunnel/engine/e2e/JobExecutionIT.java:166`
**Problem Description**:
This PR fixes a shell script layer bug but does not add any automated tests
to verify the fix. This means:
1. Cannot prevent future regressions
2. Cannot verify the effectiveness of the fix in various scenarios
3. Code reviewers cannot confirm the correctness of the fix through tests
**Potential Risks**:
- Risk 1: Future code modifications may unintentionally break this fix
- Risk 2: Some edge cases may not be covered by tests, causing production
environment issues
**Impact Scope**:
- Direct impact: Code quality and long-term maintainability
- Indirect impact: Users may encounter this issue again in future versions
- Affected area: Core startup scripts
**Severity**: MINOR
**Improvement Suggestions**:
It is recommended to add integration tests to verify parameter passing:
```java
// Add test file: SeaTunnelClientArgParsingIT.java
@Test
public void testJobNameWithSpaces() throws Exception {
// Call seatunnel.sh via ProcessBuilder
// Verify job name is passed correctly
}
@Test
public void testConfigPathWithSpaces() throws Exception {
// Test config file path with spaces
}
@Test
public void testVariableWithSpaces() throws Exception {
// Test -i parameter with values containing spaces
}
```
Or at least add a simple shell script test:
```bash
# Add test script: seatunnel-starter/src/test/bin/test_args_parsing.sh
#!/bin/bash
# Verify parameters are correctly passed to Java
source seatunnel.sh
# Test parameters with spaces
```
**Reasons**:
- Automated testing is key to preventing regressions
- Test coverage of edge cases can improve code quality
- For shell script bugs, testing is particularly important (because shell
scripts are prone to unexpected side effects)
---
### Issue 3: Documentation and CHANGELOG not updated
**Location**:
- Missing updates: Related documentation under `docs/` directory
- Missing updates: `CHANGELOG.md` (if the project maintains this file)
**Problem Description**:
This fix is a user-visible improvement (job names can now contain spaces),
but it is not documented. Users may not know about this fix, or may mistakenly
think that spaces are not supported when encountering issues.
**Potential Risks**:
- Risk 1: Users may still avoid using job names with spaces, unaware that it
has been fixed
- Risk 2: The upgrade guide does not mention this fix, which may lead to
unexpected behavior after upgrading
- Risk 3: Future maintainers may not know why the code was modified this way
**Impact Scope**:
- Direct impact: User experience and documentation completeness
- Affected area: All users using seatunnel.sh
**Severity**: MINOR
**Improvement Suggestions**:
1. Add examples in relevant documentation:
```markdown
## Submitting a Job
You can now use job names with spaces:
```bash
./bin/seatunnel.sh -m cluster -c config.conf -n "My Job Name"
```
2. 在 CHANGELOG 中添加条目：
```markdown
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
