You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/11/18 13:42:29 UTC
[GitHub] [accumulo] dlmarion opened a new issue, #3086: Ignore generated code in GitHub diffs
dlmarion opened a new issue, #3086:
URL: https://github.com/apache/accumulo/issues/3086
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
--
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: notifications-unsubscribe@accumulo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320289091
> 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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
cshannon commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320287421
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320087679
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320076223
I tested the solution to this in #3081 and it looks good to me...
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
cshannon commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1322048773
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 is that 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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion closed issue #3086: Ignore generated code in GitHub diffs
URL: https://github.com/apache/accumulo/issues/3086
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
brianloss commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1323724343
> 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](https://hub.docker.com/_/maven) for an idea of what I'm thinking). This would not preclude someone from building Accumulo in the way that they want.
Re: barrier to entry... What @dlmarion suggests could also be used to set up [codespaces](https://github.com/features/codespaces). Codespaces are free for open source development, and as far as I can tell, are enabled on the Apache GitHub org. You can work entirely in a browser if you want (though at least Visual Studio Code and JetBrains IDEs are supported too for remote development in the container), so there is no requirement to install software if you just want to play around a little bit.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1321973561
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](https://hub.docker.com/_/maven) for an idea of what I'm thinking). This would not preclude someone from building Accumulo in the way that they want.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320331440
sure, but we could just change it from pre-release to when integration tests are done.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1324151467
Draft PR up that contains the beginning of what I was thinking.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1323053172
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320116503
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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320182347
> 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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320291290
I don't understand how # 1 is a concern if the code in that directory is deleted and then regenerated at release.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3086: Ignore generated code in GitHub diffs
Posted by GitBox <gi...@apache.org>.
cshannon commented on issue #3086:
URL: https://github.com/apache/accumulo/issues/3086#issuecomment-1320300883
> 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 change) 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.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org