DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5488719462
@luozihen Thanks for flagging this, and no worries about the noise.
Before answering the revert-vs-rebase question I dug into the actual CI
failure, because fixing the merge commit alone will **not** fix CI here.
**The real root cause is not the upstream `dev` merge — it's a HOCON syntax
bug in your own new test**,
`ConfigShadeTest#testDecryptPreservesSpecialCharacterKeys`
(`seatunnel-core/seatunnel-core-starter/src/test/java/org/apache/seatunnel/core/starter/utils/ConfigShadeTest.java:407-416`
at your current head). I compared this method between your pre-merge commit
(`88c2358193b9`) and the merge commit (`2acb55a048d0`) and it is byte-for-byte
identical in both — so the `apache:dev` merge did not introduce this failure,
it was already broken before you merged.
The failing job is `unit-test` (all four JDK/OS matrix lanes), failing in
module `seatunnel-core-starter`:
```
[ERROR] Tests run: 13, Failures: 0, Errors: 1, Skipped: 0 <<< FAILURE! - in
org.apache.seatunnel.core.starter.utils.ConfigShadeTest
[ERROR] testDecryptPreservesSpecialCharacterKeys Time elapsed: 0.004 s <<<
ERROR!
org.apache.seatunnel.shade.com.typesafe.config.ConfigException$Parse:
String: 2: Expecting close brace } or a comma, got '='
at
ConfigShadeTest.testDecryptPreservesSpecialCharacterKeys(ConfigShadeTest.java:442)
```
The raw HOCON literal the test builds its input from is:
```java
"source { FakeSource { plugin_output = \"fake\" } }\n"
+ "sink { Jdbc { url = \"jdbc:mysql://localhost:3306/db\" "
+ "username = \"u\" password = \"p\" "
+ "multi-table_config { primary_keys { "
+ "\"^t_nova_.*$\" = [\"${primary_key}\", \"DATA_SOURCE\"] "
+ "\"^t_tyuen_txn_.*$\" = [\"id_txn_ctrl\", \"DATA_SOURCE\"] "
+ "} } } }"
```
In HOCON, fields inside an object must be separated by a comma or a newline.
`url = "..." username = "u"` on one physical line with only a space between
them is not valid — the parser tries to concatenate the quoted `url` value with
the following unquoted token `username`, then errors out on the `=` that comes
right after it (hence "Expecting close brace } or a comma, got '='"). The same
missing-separator problem repeats for `username`/`password`,
`password`/`multi-table_config`, and the two `primary_keys` entries
(`"^t_nova_.*$" = [...]` next to `"^t_tyuen_txn_.*$" = [...]`).
Fix: add commas (or real newlines) between every field, e.g.:
```java
Config input =
ConfigFactory.parseString(
"source { FakeSource { plugin_output = \"fake\" } }\n"
+ "sink { Jdbc { url =
\"jdbc:mysql://localhost:3306/db\", "
+ "username = \"u\", password = \"p\", "
+ "multi-table_config { primary_keys { "
+ "\"^t_nova_.*$\" = [\"${primary_key}\",
\"DATA_SOURCE\"], "
+ "\"^t_tyuen_txn_.*$\" = [\"id_txn_ctrl\",
\"DATA_SOURCE\"] "
+ "} } } }");
```
(or switch to a Java text block / genuinely multi-line literal so real
newlines do the separating for you — that tends to be less error-prone than
tracking commas across a long chain of concatenated strings).
On your actual question, once that test is fixed:
- **Recommendation: rebase (your option 2), not revert.** This is your own
PR source branch, not a shared/protected branch, so an interactive rebase to
drop the accidental merge commit followed by a force-push is completely normal
and safe here — that's exactly what rebasing your own feature branch is for.
`git revert -m 1 <merge-sha>` also "works," but reverting a merge commit has a
well-known follow-up trap: if you ever merge `apache:dev` into this branch
again later, git can silently treat the commits from the first merge as
already-integrated-then-reverted, and you'd have to revert your own revert
first to bring them back. Rebasing avoids that trap and gives reviewers a
clean, linear diff.
- Practically, it won't affect the permanent project history either way:
Apache SeaTunnel merges PRs with squash-merge, so everything on this branch
collapses into a single commit on `dev` regardless of whether the branch itself
contains a merge commit, a revert commit, or a clean rebase.
So: rebase onto the latest `apache:dev`, fix the missing commas in
`testDecryptPreservesSpecialCharacterKeys`, force-push to your own branch, and
CI should go green. Let me know if anything else comes up.
--
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]
