sharpca commented on PR #13081:
URL: https://github.com/apache/seatunnel/pull/13081#issuecomment-6607321549
Thanks @DanielLeens, and thanks for the correction on both points.
**CI**
You are right and I had it wrong: I was reading `action_required` on the
Build check as an approval gate that a maintainer had to clear, and it is not.
The build has to run in `sharpca/seatunnel`, that fork has no workflow runs at
all, and that is why the check reports "Workflow run detection failed". So
there is nothing for anyone to approve, and the ball is in my court:
1. enable Actions on the fork,
2. run the `Build` workflow for this branch (or push an empty commit),
3. let the PR check adopt the fork run.
I will do that for #13081 and for #13083. If the check still reports
"Workflow run detection failed" after the fork run exists, I will reply here
with the run id rather than assume it is a flake.
**`--checkpoint-history-status` — agreed, and I will fix it in this PR**
It is the same class of bug as the issue title, so it belongs here rather
than in a follow-up. `ClientExecuteCommand` currently does:
```java
status = CheckpointStatus.valueOf(
clientCommandArgs.getCheckpointHistoryStatus().toUpperCase());
```
Under tr-TR, `failed` upper-cases to `FAİLED` (dotted `İ`, `U+0130`),
`valueOf` throws, and the branch reports "Unsupported checkpoint history
status" for an input that is valid everywhere else. The fix is the same shape
as the rest of the issue: `toUpperCase(Locale.ROOT)`, pulled into a small
`parseCheckpointStatus(String)` helper so it can be asserted directly instead
of through a client that needs a cluster — `ClientExecuteCommand#execute`
builds a `SeaTunnelClient`, so a test that goes through `execute()` would be an
integration test for one string conversion.
One correction to your message while I am here: `CheckpointStatus` has four
constants, not two — `IN_PROGRESS`, `COMPLETED`, `FAILED`, `CANCELED`. The new
test covers all four under tr-TR (and asserts that an unknown value still
throws), so the assertion pins the whole enum rather than the two values that
happen to be in use today. `seatunnel-starter` already has JUnit 5 tests in the
same package (`ApplicationExecuteCommandTest`, `ConnectorCheckCommandTest`,
...), so no build change is needed.
**On the other `seatunnel-core` conversions**
Agreed that `AbstractFlinkRuntimeEnvironment` (checkpoint mode, time
characteristic) reads job config rather than CLI arguments and is out of scope
for this issue. I will not touch it here; if it deserves a fix it should be its
own issue, since the trigger is a config file value rather than an argument a
user types.
Once the fork build has run I will ping here with the result, and the
detailed review can continue on #13081.
--
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]
