DanielCarter-stack commented on PR #10495:
URL: https://github.com/apache/seatunnel/pull/10495#issuecomment-3901323847
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10495", "part": 1,
"total": 1} -->
### Issue 1: Commit History Chaos
**Location**: Git commit history
**Related Context**:
```
03e67e0e6: .github/workflows/backend.yml 添加 DOCKER_API_VERSION: 1.44
a31b60c81: 改为 MAVEN_OPTS: "-Dapi.version=1.44"
1231917cf: 改为 MAVEN_OPTS: "-Ddocker.client.api.version=1.44
-Dapi.version=1.44"
8c079baff: 移除backend.yml配置，改为pom.xml配置
71136904f: 添加surefire的api.version
```
**Problem Description**:
The PR contains 5 commit histories with identical titles but different
content. This is a typical case of "not cleaning up commit history after trying
multiple solutions." The correct approach should be:
1. Use `git rebase -i` to merge these commits into one
2. Or use `git reset` to recreate a clean commit
3. Commit messages should accurately reflect the content of each change
**Potential Risks**:
- Risk 1: History is chaotic after merge, difficult to trace back
- Risk 2: If revert is needed, multiple commits may need to be reverted
- Risk 3: Code review is difficult, cannot determine which commit is the
final solution
**Impact Scope**:
- Direct impact: Git history, code review process
- Indirect impact: Future maintainers' understanding cost
- Impact area: Git repository history
**Severity**: **CRITICAL**
**Improvement Suggestions**:
```bash
# Developers are recommended to execute before merging:
git reset dev
# Recreate a clean commit
git add pom.xml
git commit -m "[Hotfix][CI] Force Docker API version to 1.44 for
Testcontainers
Set api.version=1.44 in maven-surefire-plugin and maven-failsafe-plugin
to fix CI failures caused by Docker API version mismatch.
Ref: https://github.com/testcontainers/testcontainers-java/issues/11491";
```
**Rationale**:
Merging multiple exploratory commits is a standard practice that makes
history clearer. Commit messages should include context information, not just
titles.
---
### Issue 2: Missing Upgrade Path Documentation
**Location**: `pom.xml:669` and `pom.xml:700`
**Related Context**:
- Testcontainers issue #11491
- Testcontainers current version: 1.17.6 (pom.xml:141)
**Problem Description**:
The comment is marked as "temporary fix" but does not specify:
1. When this configuration can be removed (which Testcontainers version will
fix it?)
2. How to monitor Testcontainers updates?
3. Who is responsible for removing this configuration?
**Potential Risks**:
- Risk 1: This temporary configuration may remain in the code permanently
- Risk 2: This configuration may become redundant or even harmful after
Testcontainers upgrade
- Risk 3: Future maintainers won't know why this configuration is needed
**Impact Scope**:
- Direct impact: pom.xml configuration maintenance
- Indirect impact: Testcontainers version upgrade process
- Impact area: Build configuration
**Severity**: **MAJOR**
**Improvement Suggestions**:
```xml
<!-- Temporary fix for
https://github.com/testcontainers/testcontainers-java/issues/11491 -->
<!--
FIXME: Remove this when Testcontainers is upgraded to 1.18.0 or higher
Expected fix version: Testcontainers 1.18.0+
Tracking issue:
https://github.com/testcontainers/testcontainers-java/issues/11491
Date added: 2026-02-14
-->
<api.version>1.44</api.version>
```
Or create a JIRA task to track:
```xml
<!--
Temporary workaround for Docker API version mismatch in GitHub Actions
See: https://github.com/testcontainers/testcontainers-java/issues/11491
Remove when: TESTCONTAINERS-XXX is resolved
-->
<api.version>1.44</api.version>
```
**Rationale**:
Clear upgrade path and responsibility assignment help with technical debt
management. Marking FIXME and expected version lets future maintainers know
when this configuration should be removed.
---
### Issue 3: Missing Testcontainers Version Upgrade Assessment
**Location**: `pom.xml:141`
**Related Context**:
- Current version: `<testcontainer.version>1.17.6</testcontainer.version>`
- Testcontainers latest version (as of February 2026): possibly 1.19.x or
higher
**Problem Description**:
The PR chose to set `api.version=1.44` as a temporary solution, but did not
assess whether Testcontainers version should be upgraded directly. Upgrading
Testcontainers could:
1. Natively support new Docker API
2. Include other bug fixes and performance improvements
3. Avoid hardcoding version numbers
**Potential Risks**:
- Risk 1: Missing other improvements from upgrading Testcontainers
- Risk 2: 1.17.6 may have known security vulnerabilities
- Risk 3: Temporary fix may become invalid after Testcontainers upgrade
**Impact Scope**:
- Direct impact: Dependency management strategy
- Indirect impact: Test stability and security
- Impact area: All tests using Testcontainers
**Severity**: **MAJOR**
**Improvement Suggestions**:
Add explanation in PR description or comments:
```markdown
### Why not upgrade Testcontainers?
Considered upgrading from 1.17.6 to latest, but decided not to because:
- [x/y] Breaking changes in newer versions would require extensive test
updates
- [x/y] Need to verify compatibility with all connectors
- [x/y] Temporary fix is lower risk for now
Plan to upgrade Testcontainers in separate PR: [issue link]
```
Or if upgrade is feasible, should upgrade:
```xml
<!-- Upgrade from 1.17.6 to 1.19.0 to fix Docker API compatibility -->
<testcontainer.version>1.19.0</testcontainer.version>
```
**Rationale**:
Technical debt management needs to consider long-term maintenance costs. If
the cost of upgrading Testcontainers is acceptable, upgrading should be
prioritized over patching. Even if choosing a temporary solution, reasons for
not upgrading should be documented, and future upgrades should be planned.
---
### Issue 4: Local Development Environment Compatibility Not Considered
**Location**: `pom.xml:669-670, 700-703`
**Related Context**:
- Local development may use older Docker (e.g., Docker Desktop 20.10.x, API
1.41)
- Docker versions on macOS and Linux may differ
**Problem Description**:
Hardcoding `api.version=1.44` may cause local developer tests to fail if
local Docker version doesn't support 1.44. No documentation explains:
1. What Docker version is required for local development?
2. What to do if local Docker version doesn't match?
3. Can this configuration be disabled for local development?
**Potential Risks**:
- Risk 1: Developers cannot run tests locally, wasting time troubleshooting
- Risk 2: May force developers to upgrade Docker, affecting other local
projects
- Risk 3: Reduces development experience
**Impact Scope**:
- Direct impact: Local development workflow
- Indirect impact: Development efficiency
- Impact area: All contributors
**Severity**: **MINOR**
**Improvement Suggestions**:
Add explanation in project documentation (such as CONTRIBUTING.md or
development documentation):
```markdown
### Docker Version Requirements
Integration tests require Docker API 1.44 or higher.
- CI/CD: Uses Docker 24.x (API 1.44)
- Local development: Requires Docker Desktop 24.x+ or Docker Engine 24.x+
If your local Docker version is older:
1. Upgrade Docker Desktop/Engine
2. Or skip Docker-based tests: `mvn verify -DskipIT=true`
3. Or override the API version: `mvn verify -Dapi.version=1.41`
```
Or a more flexible approach (not recommended as it reduces consistency):
```xml
<!-- Only enforce API version in CI environment -->
<api.version>${env.DOCKER_API_VERSION,1.44}</api.version>
```
**Rationale**:
Although this is a temporary fix, impact on all development environments
should be considered. Clear documentation can reduce developer confusion.
---
### Issue 5: No Docker Version Check Added
**Location**: `.github/workflows/backend.yml` (removed configuration)
**Related Context**:
- GitHub Actions environment Docker version may change over time
- First commit (03e67e0e6) attempted to set at GitHub Actions level
**Problem Description**:
No explicit check for GitHub Actions Docker version, relying on
Testcontainers error message to discover incompatibility. Better practice in CI
configuration:
1. Explicitly specify Docker version
2. Or check Docker version before tests
**Potential Risks**:
- Risk 1: If GitHub Actions upgrades Docker to higher version, 1.44 may not
be enough again
- Risk 2: Difficult to quickly diagnose Docker-related issues
**Impact Scope**:
- Direct impact: CI stability
- Indirect impact: Issue diagnosis speed
- Impact area: CI pipeline
**Severity**: **MINOR**
**Improvement Suggestions**:
Add Docker version check in `.github/workflows/backend.yml`:
```yaml
- name: Check Docker version
run: |
DOCKER_VERSION=$(docker version --format '{{.Server.APIVersion}}')
echo "Docker API Version: $DOCKER_VERSION"
if [[ $(echo "$DOCKER_VERSION 1.44" | tr " " "\n" | sort -V | head -n1)
!= "1.44" ]]; then
echo "Error: Docker API version $DOCKER_VERSION is less than 1.44"
exit 1
fi
```
Or print Docker version information at job start:
```yaml
env:
TEST_IN_PR: ${{ inputs.TEST_IN_PR }}
# Explicitly document Docker version requirement
REQUIRED_DOCKER_API_VERSION: "1.44"
steps:
- name: Print Docker info
run: |
docker version
docker info
```
**Rationale**:
Explicit checking can discover issues before they occur, rather than waiting
for Testcontainers to error. This improves CI robustness and maintainability.
---
### Issue 6: Insufficient PR Description
**Location**: GitHub PR page
**Related Context**:
- PR description has only simple error message and issue link
- Missing reproduction steps, testing methods, etc.
**Problem Description**:
PR description does not include:
1. Why are there 5 commits with identical titles?
2. Why choose Maven configuration over other solutions?
3. How to verify locally?
4. Besides Testcontainers issue, are there related SeaTunnel issues?
**Potential Risks**:
- Risk 1: Reviewers need to check git history to understand complete context
- Risk 2: Future maintainers won't know the complete background of this PR
**Impact Scope**:
- Direct impact: Code review quality
- Indirect impact: Knowledge transfer
- Impact area: Project documentation
**Severity**: **MINOR**
**Improvement Suggestions**:
PR description should supplement:
```markdown
## Background
CI started failing consistently with error:
```
client version 1.32 is too old. Minimum supported API version is 1.44
```
This is tracked in
https://github.com/testcontainers/testcontainers-java/issues/11491
## Root Cause
GitHub Actions upgraded Ubuntu to latest, which includes Docker 24.x with
minimum API 1.44.
Our Testcontainers version (1.17.6) defaults to API 1.32.
## Solution Evaluated
1. Upgrade Testcontainers to latest: Too many breaking changes, risky
2. Set DOCKER_API_VERSION in GitHub Actions: Not propagated to tests
3. Set api.version in Maven plugins: ✅ Chosen
## Changes
- Set `api.version=1.44` in both maven-surefire-plugin and
maven-failsafe-plugin
- This forces Testcontainers to use Docker API 1.44
## Testing
- [x] CI backend tests pass
- [x] Local tests pass (requires Docker 24.x+)
- [ ] Add link to successful CI run
## Follow-up
- Create issue to upgrade Testcontainers to 1.18.x when available
- Remove this temporary fix after Testcontainers upgrade
```
**Rationale**:
Detailed PR description is an important part of code review. It helps
reviewers quickly understand the background and facilitates future reference.
---
--
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]
