DanielLeens commented on PR #11613: URL: https://github.com/apache/seatunnel/pull/11613#issuecomment-6411599491
Thanks @SEZ9 for the pass on `117cf5289a`. As you noted, this head is only a merge of `dev` over `379efa74`. I compared the PR-owned Java, UI, test and resource files between the two heads and they are identical. That is why I re-checked each of the eight points above directly against `117cf5289a` (source read only, nothing executed locally). Several of them look already addressed in the code, so here is where each one lives, to save you a pass: - **F1**: `docs/en/engines/zeta/rest-api-v2.md` (the `/update-tags` section, around line 1400) states that `/update-tags` is the legacy flat `Map` contract and that no key, including `uuid` and `tags`, is reserved. The structured contract is documented in the separate `/update-local-member-tags` section. - **F2**: `RestApiIT#testHttpServiceStatusReportsEffectivePortsWithoutSecrets` and `RestApiIT#testUpdateLocalMemberTagsOnlyMutatesTheServingMember` cover `/http-service/status` and the serving-member and mismatching-UUID cases over real HTTP. The legacy `/update-tags` calls in the same IT cover the unchanged behavior. - **F3**: both servlets are registered on the same `ServletContextHandler` as the existing endpoints, so `BasicAuthFilter` and the mTLS connector settings apply unchanged. `BasicAuthenticationIT` also has the unauthenticated-request checks for both new endpoints. - **F4**: the `Limitation` subsection of `/update-local-member-tags` documents that the UUID check is local to the serving node. - **F5**: `/update-local-member-tags` has its own section with a body table, a success example and an error table. The non-string value case was added in `379efa74e9`, and `UpdateTagsServiceTest` covers it. - **F6**: `web-ui.md` (Checkpoints bullet) distinguishes an active source job's HTTP 400 from the `No checkpoint found` result after checkpoint cleanup. - **F7**: `/http-service/status` has a per-field table and the example uses port 8080. The only item that is really still open is **F8** (screenshots for the new UI surfaces). I treat it as Low severity and non-blocking, and @danielnadean has already said it is a documentation follow-up. If you would prefer it split into its own PR, that seems reasonable to me. The remaining real gate is unchanged from my last review: the PR is still a Draft and the `Build` check is `ACTION_REQUIRED`, because fork Actions are disabled and no workflow has run on this head. Once a full `Build` result exists I will compare any failure against the diff. Thanks again to everyone on this thread. -- 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]
