Ignore generated code in GitHub diffs

[dlmarion]

It appears that we can mark certain paths to be hidden by default in diffs and to not count against the repository's language statistics. See https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github

[dlmarion]

I tested the solution to this in #3081 and it looks good to me...

[ctubbsii]

That's very cool. I didn't know about this.

Some points to consider:

1. This makes it easier for unreviewed malicious code to sneak in via an innocuous pull request; we still have to review the contents of these directories before merging if they appear in a PR, and
2. GitHub's UI shows these suppressed files in a format that is very similar to their automatic suppression of large diffs that still need to be reviewed, so you still have to scroll past them in the UI and read the message for each one to determine if they need to be reviewed individually.
3. Since this only affects how generated files are displayed in GitHub, it's still nice to have a separate commit in the git history for when one is reviewing the logs on the command-line or in other git tools.

GitHub's UI for this could be a little better. I'd rather have toggle to automatically hide/show all generated files, similar to GitHub's hide/show whitespace changes toggle. Also, I'd really like the generated code to be in a separate commit for the command-line history when we make big changes to the thrift files.

My conclusion: I think using this is probably better than not using it, but it doesn't help with everything we care about.

[dlmarion]

re point 1 above - I wonder if we can reduce some of the risk by removing all of the files in this directory and regenerating them during the release process.

[ctubbsii]

re point 1 above - I wonder if we can reduce some of the risk by removing all of the files in this directory and regenerating them during the release process.

That's already done. The release process activates the thrift profile. So, the main risk is to the developers prior to release.

[cshannon]

This is definitely a nice feature but as stated, number 1 is my main concern too. Would be easy to sneak in something or even just screw up the generated code by mistake and might get missed.

Also, when I view #3081 now it doesn't seem like there is an easy way to still view the diff for the generated code with a toggle option so it seems like it's completely hidden and no way to easily view it if you wanted which is kind of annoying.

[ctubbsii]

it doesn't seem like there is an easy way to still view the diff for the generated code with a toggle option

You have to click on each "Load diff" link for each separate file. You can't do it for all at once.

[dlmarion]

I don't understand how # 1 is a concern if the code in that directory is deleted and then regenerated at release.

[cshannon]

I don't understand how # 1 is a concern if the code in that directory is deleted and then regenerated at release.

Well one issue could be is that there would be a long delay from when the issue was introduced vs catching it (if we don't do releases for a long time). So we could be potentially developing/testing against something that is not what will be generated later. And (in the unlikely chance) there was something introduced that was malicious it would be loaded on any developer machines still.

I wonder if there is some way to auto detect changes to the directory when unintentional (ie we didn't run thrift). Not sure if there is some way to mark a directory as read only in Git or use some kind of git post commit hook or add something in maven to detect changes, etc. Maybe we could do something with Jenkins/CI to detect changes and report so we catch it.

[dlmarion]

sure, but we could just change it from pre-release to when integration tests are done.

[ctubbsii]

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.

[dlmarion]

I feel like the simple answer here is to just remove the Thrift profile and run Thrift during the compile phase. It only takes a couple of seconds and is dwarfed by the time it takes to run tests. Removing the contents of the directory that contain the Thrift generated code and regenerating the code should alleviate the concern about unreviewed malicious code being introduced into that directory. Using the .gitattributes feature I found will hide the thrift generated java files during the PR, and I think we can safely ignore them during a review.

It may be the case that not everyone has Thrift installed, and I agree that it's not the easiest thing to install. I'm thinking that for this issue we create a Docker build environment that can be used locally and by the automated build systems. The Docker image would have Java, Thrift, Maven, etc installed and would build the code that is checked out on the local filesystem (see this for an idea of what I'm thinking). This would not preclude someone from building Accumulo in the way that they want.

[cshannon]

If you did exclude it from the build then you wouldn't even need the .gitattributes at all as you would just ignore the generated code in git entirely and not commit it so it wouldn't be part of the commit/repository.

While normally I would say that's the way to go (just generate each time and not commit) I don't know that I like the idea of someone having to set up Thrift to build or Docker or whatever. Adding that extra dependency is a bit annoying vs just being able to checkout and build using Maven. But this may be the best option as long as it's pretty quick to generate the Thrift code, we can just have instructions for setting up Thrift and for also using Docker.

Something else I thought of as an alternative...this has been making me wonder if if it makes sense to just pull all of the Thrift stuff into it's own separate project and make it a separate dependency that the main project depends on. Not saying we necessarily want to do that but it's another option. I guess it mostly depends on how often we change the protocol, if there's a lot of changes then it would get annoying to have to keep updating another project and releasing it so may or may not be the right answer.

One other benefit of moving it to a separate project is that there has been some initial discussion about having a plugable protocol at some point. Right now we require Thrift and it's tightly coupled but we don't have to in the future so if we decided to refactor it to have an API and allow people to use a different RPC protcol (something like grpc and protofbuf for example) having it separated out makes sense too.

[ctubbsii]

One of the first tickets I worked on when the project went open source was https://issues.apache.org/jira/browse/ACCUMULO-756. That never got merged, but I think it can/should be revisited. It would make it easier to put the thrift stuff in its own project. However, that's a much bigger task than what's being discussed here, and even that doesn't solve the problem we're discussing much, because the changes that prompted this discussion, as well as many Thrift changes involving generated code changes, are changes where we need to change the RPC layer to support some feature on top, so both layers are changing simultaneously.

I share the concerns about raising the barrier for new folks. Requiring docker instead of thrift isn't much better. I'm not entirely opposed to dropping the generated code from the source tree and just always activate the thrift profile, so it is generated each build, but I'd lean towards just keeping the status quo as my first choice. My second choice would be to do the .gitattributes thing and just deal with the generated code being squashed into the same commits as the changed code that generated it, as proposed here.