ctubbsii commented on issue #3086: URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320927753
The main point for items 1 and 2 is that it puts responsibility on the committer who does the merge to ensure they've done their due diligence before merging. One way they can ensure the code is good is to just check out the pull request, delete the generated directory, and build with `mvn package -Pthrift -DskipTests` to verify that the contents of the generated directory are the same using their local build as what is in the PR. I normally do this anyway, but it saves me steps when we merge a PR that omits those changes, and I just commit what I generate locally. For what it's worth, it is extremely rare for a non-committer to touch thrift... as that generally requires expertise and a developer environment that most new contributors don't have yet. And, for committers, who who we already trust with write access to the repo, this isn't really an issue. Item 2 is just suboptimal GitHub tooling. Item 3 is my biggest concern, because looking at changes in the history is very inconvenient when the thrift generated code is squashed into the non-generated code in the same commit, and the generated code diff drowns out the important diffs. In my mind, it's just as easy to just omit the generated code from the PR to begin with, and apply those changes later... but then you don't get the automated checks if it doesn't compile (unless we configure GitHub Actions to build thrift). One way this could be done is to have a PR that omits the generated code, then a separate PR that depends on it that includes it. We can review the one that omits it, and rely on the automated checks from the PR that includes it. But then there's a risk that they could be different. Another option is to just stop checking in generated code to the repo, and just make that part of the build. However, that also is troublesome, because thrift is a pain to build (we can't just run a code generator as a maven plugin), and because the code needs to be generated prior to loading the project into an IDE. None of the available options are perfect. We just have to find the least bad one. -- 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]
