nevzheng commented on PR #12389: URL: https://github.com/apache/gravitino/pull/12389#issuecomment-5259661398
@roryqi @Octavi00 Logging my recommendation so the decision is clear. I would approve this PR as-is, merge it, and move on. On the binary fixtures: I do not think generating them at test setup adds enough value to justify reworking this PR. The maintainability concerns (provenance, regeneration, inspectability) are already addressed by [`regenerate.sh`](https://github.com/Vonna9/gravitino/blob/fcb8bd57c3dc94e6f2361a5c5915c39fc4919559/server-common/src/test/resources/tls/regenerate.sh) and the [`README`](https://github.com/Vonna9/gravitino/blob/fcb8bd57c3dc94e6f2361a5c5915c39fc4919559/server-common/src/test/resources/tls/README.md). Reviewers should review that generation path and docs, not the `.p12` diffs. If we still want automated generation, please do it as a follow-up PR. That keeps this M1 diff focused and avoids delaying work that has already been blocked for a long time. Part of our responsibility as reviewers is to keep work moving. Prefer comments that make the change [materially better](https://google.github.io/eng-practices/review/reviewer/standard.html) — favor approving once a CL clearly improves the system, even if it is not perfect, and balance the importance of the suggestion against the need to make forward progress. For me, runtime fixture generation does not clear that bar relative to the delay cost on this milestone. -- 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]
