Octavi00 commented on PR #12389: URL: https://github.com/apache/gravitino/pull/12389#issuecomment-5248281337
> Thanks for the explanation. > > My concern is mainly about long-term maintainability. Binary fixtures are hard to review, diff, and version over time. If a certificate needs to be updated later, reviewers cannot easily tell what changed or whether the generated content still matches the documented intent. > > I agree that committing the binary files is simpler for this PR, but I do not think it is a good practice for this repo. Since the PR already includes a regeneration script and README, I would prefer that we keep the source/configuration for generating the certificates in the repo and generate the binary artifacts during the test/setup process instead of committing them directly. Hi @roryqi, I see what you mean, as in you can't easily read a binary file as a reviewer. I'm assuming it's probably from a security standpoint of the repo that you don't want to accept an unreadable file from a PR. I think the simplest fix, if you are willing to keep a binary implementation, would be to accept my PR without the trustores and submit your own pr after inspecting my regen script and running it on your end, then uploading the result. Before I had decided to use a stable fixture, I had attempted to implement generating a runtime trust store, but I had inquired if it was necessary after realizing it would take me some more time to research and learn what a good method would be. As of right now, I wouldn't recommend generating the test trust stores at runtime by trying to implement my regen script, as you would need to import X.509 api's, which would introduce a large dependency. I would also avoid using a bouncyhouse implementation too, simply because I think it would be possible to avoid adding any dependency at all if we use the JDK's keytool, which I'm already using to create the licenses for tests. Using keytool, I could implement generating a runtime trust store within my Util file pretty cleanly since it's already getting published and available across different test classes. The only downside is the fact that it would theoretically slow down tests, as they would need to be generated at least once per test class. I'm currently going to start working on M2 in the meantime, but if you'd like me to try the keytool approach, I could make the change. -- 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]
