-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider using gerrit for code reviews #136
Comments
I'm gonna vote 👎 on this, but let's see what happens when we get people fully staffed. Putting more tooling in the way of outside contributions is :( , particularly when Github's PR system is commonly accepted in the open source world. |
I find gitHub code review tool to be much inferior compared to gerrit. The lack of 2 pane diff mode, the embedded comments (which can disappear without a trace based on some code change), the lack of marking what was already viewed, horrible history navigation and most of all the immediate comment notification makes it a very hard to work with tool. I am not sold about gerrit, only had a recent experience with it and realized how much better it is compared to gitHub's code review. If there are other better code review tools that work well with gitHub I think gcloud-golang is also using gerrit, no? Does it negatively effect their outside contribution? |
I believe gerrithub will create a gerrit CL when someone creates a pull On Thu, Aug 20, 2015 at 4:30 PM, JJ Geewax notifications@github.com wrote:
|
Hard to say whether Gerrit negatively influences outside contributions. We |
Totally agree with the complaints -- but this project is about meeting our users where they are and using the tools they use.
I personally love gerrit... I used it with a previous company after we found ReviewBoard to be kind of annoying, but my 👎 vote sadly still stands...
They do, however I don't think we have enough data to say yes or no. If I had to place a bet on this.... I'd say the typical contributor would be annoyed with needing to do anything extra to contribute... @broady: If someone creates a PR inside Github (aka, doesn't do anything with gerrit), what happens? |
/cc @GoogleCloudPlatform/gcloud @ryanseys @blowmage @stephenplusplus @dhermes |
@broady Do you mean we use gerrit for all Google-owned Go projects? Or that it's the standard for all open-source Go projects? |
/cc @mziccard - Any thoughts on this? |
/cc @jboynes - Opinon ? |
What happens in gcloud-golang? Nothing. The PR just sits there. It doesn't If gerrithub was linked, I think it creates a Gerrit CL. Gerrit is the standard for open-source Go projects, like the Go project |
So you guys can't do a code review inside just Github ? To contribute you really need to go the gerrit route, correct?
Gotcha -- thanks @broady . That makes sense. I don't believe there's really a standard in the Java world, so I think we have to actually have to give this some serious thought, where, lacking compelling reasons, the default here is stick with Github's tooling given our audience. |
It's not that we "can't", it's that we won't. It's an awful experience. We tell people to use Gerrit instead. |
Cool - thanks @dsymonds I agree that gerrit is a much happier experience :( but I'd still vote for the GH tooling simply due to momentum. |
I don't personally care what gcloud-java does. If GitHub pull requests are the standard for the relevant community, and the heaviest users prefer it, I'd say go for it. |
Here's another one: https://reviewable.io/ -- but I haven't used either. I think the GH interface for reviews is sub-optimal, but I'm not sure having to incorporate another tool is worth it. I would rather let our communities start the trend and have us follow. And if that's the case for a certain language, that's fine. I don't think all of gcloud-* have to be consistent in process. Do what works for your team. As far as the Node world, I haven't personally been a part of a project that preferred code reviews to be done off-site. If I was just stopping by to make a one-off contribution, I wouldn't be thrilled to have to learn something new. If we're just concerned with reviewing each others code, again, do whatever the team prefers. |
@aozarov 2-pane diff mode: https://github.com/GoogleCloudPlatform/gcloud-node/pull/94/files?diff=split :) |
I don't have a strong opinion on the matter. We can all agree that Gerrit has a better experience wrt Github's PR. However, I have the feeling (no evidence whatsoever) that the users that are familiar with Github's PR are much more than the users familiar with Gerrit. I just had a look at Gerrithub and I don't think that a PR in Github triggers a Gerrit CL (as I hoped) but please correct me if I am wrong. |
I've used Gerrit at other jobs and for contributing to OpenStack, and I'm a 👎 on requiring it for open source contributions. I haven't found the advantages to be worth the cost. But I'm also a filthy rubyist so you can safely disregard my opinion. :) |
I strongly feel that requiring Gerrit for contributions would negatively impact Ruby community participation in the project. |
For example -- we'd lose people making docstring changes. I know if I can fork -> click the edit button -> send pull request, I'm much more likely to submit a change. If I'm required to hit the command line... no thanks. |
One more data point: So I created PR for gcloud-golang for the CoC. A CL was not created in Gerrit for my PR. Then I was told, in the nicest way possible, that the contribution didn't count and they had to do it over. The CL was created manually, and there is no attribution given for the contribution. So, if I was unfamiliar of the Gerrit requirement, the end result for my contribution is someone else gets to take credit for it and I get to feel dumb about it. This is why I do not believe this is not a good experience for bringing outside folks into open source projects. |
This PR has been open for a while. It seems from the discussion that we should stick to github's suboptimal code review in order to favor user contributions. If you feel we should give this more thoughts feel free to reopen. |
- [ ] Regenerate this pull request now. PiperOrigin-RevId: 454027580 Source-Link: googleapis/googleapis@1b22277 Source-Link: https://github.com/googleapis/googleapis-gen/commit/e04cea20d0d12eb5c3bdb360a9e72b654edcb638 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZTA0Y2VhMjBkMGQxMmViNWMzYmRiMzYwYTllNzJiNjU0ZWRjYjYzOCJ9
…onfig to v0.5.0 (googleapis#136) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [com.google.cloud:google-cloud-shared-config](https://github.com/googleapis/java-shared-config) | minor | `0.4.0` -> `0.5.0` | --- ### Release Notes <details> <summary>googleapis/java-shared-config</summary> ### [`v0.5.0`](https://github.com/googleapis/java-shared-config/blob/master/CHANGELOG.md#​050-httpswwwgithubcomgoogleapisjava-shared-configcomparev040v050-2020-04-07) [Compare Source](https://github.com/googleapis/java-shared-config/compare/v0.4.0...v0.5.0) ##### Features - add ban duplicate classes rule ([#&googleapis#8203;126](https://www.github.com/googleapis/java-shared-config/issues/126)) ([e38a7cc](https://www.github.com/googleapis/java-shared-config/commit/e38a7cc949396f8f5696e62cd060e0c076047b8d)) - add devsite javadoc profile ([#&googleapis#8203;121](https://www.github.com/googleapis/java-shared-config/issues/121)) ([7f452fb](https://www.github.com/googleapis/java-shared-config/commit/7f452fb6c5704f6ce0f731085479a28fb99ebcb9)) - add maven flatten plugin ([#&googleapis#8203;127](https://www.github.com/googleapis/java-shared-config/issues/127)) ([fb00799](https://www.github.com/googleapis/java-shared-config/commit/fb00799c416d324d68da5b660a26f7ef595c26d9)) ##### Bug Fixes - declare com.coveo:fmt-maven-plugin version/configuration ([#&googleapis#8203;90](https://www.github.com/googleapis/java-shared-config/issues/90)) ([5cf71a6](https://www.github.com/googleapis/java-shared-config/commit/5cf71a6ed699907082756e70c2fdee6ad3632f38)) ##### Dependencies - update dependency com.google.cloud.samples:shared-configuration to v1.0.13 ([#&googleapis#8203;118](https://www.github.com/googleapis/java-shared-config/issues/118)) ([58ae69e](https://www.github.com/googleapis/java-shared-config/commit/58ae69eb5ba037963cdaed0c2b0e30663bc873eb)) - update dependency com.puppycrawl.tools:checkstyle to v8.29 ([f62292d](https://www.github.com/googleapis/java-shared-config/commit/f62292dab75699a75f8a7d499fe2ccfb7ee93783)) - update dependency org.apache.maven.plugins:maven-antrun-plugin to v1.8 ([#&googleapis#8203;124](https://www.github.com/googleapis/java-shared-config/issues/124)) ([a681536](https://www.github.com/googleapis/java-shared-config/commit/a68153643400c3f3b5c5959cda4dc3b552336427)) - update dependency org.apache.maven.plugins:maven-dependency-plugin to v3.1.2 ([#&googleapis#8203;107](https://www.github.com/googleapis/java-shared-config/issues/107)) ([c9b096b](https://www.github.com/googleapis/java-shared-config/commit/c9b096b81b1f4f8dc2d1302f259f0321722e1ca5)) - update dependency org.apache.maven.plugins:maven-site-plugin to v3.9.0 ([#&googleapis#8203;103](https://www.github.com/googleapis/java-shared-config/issues/103)) ([abe7140](https://www.github.com/googleapis/java-shared-config/commit/abe714060e858c70a83888fb34438c45d97b1168)) - update dependency org.codehaus.mojo:build-helper-maven-plugin to v3.1.0 ([#&googleapis#8203;101](https://www.github.com/googleapis/java-shared-config/issues/101)) ([ac69572](https://www.github.com/googleapis/java-shared-config/commit/ac69572be76e4462fdf65fa6e7f0935c3d51d827)) </details> --- ### Renovate configuration :date: **Schedule**: At any time (no schedule defined). :vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied. :recycle: **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. :no_bell: **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#googleapis/java-resourcemanager).
🤖 I have created a release *beep* *boop* --- ## [0.4.0](googleapis/java-bigquerymigration@v0.3.0...v0.4.0) (2022-07-01) ### Features * Add SQL Server dialect to bigquerymigration v2 client library ([9adb1d6](googleapis/java-bigquerymigration@9adb1d6)) * Enable REST transport for most of Java and Go clients ([googleapis#131](googleapis/java-bigquerymigration#131)) ([b3507db](googleapis/java-bigquerymigration@b3507db)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v2.13.0 ([googleapis#130](googleapis/java-bigquerymigration#130)) ([53885d6](googleapis/java-bigquerymigration@53885d6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- ## [1.2.3](googleapis/java-ids@v1.2.2...v1.2.3) (2022-09-09) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.2 ([#135](googleapis/java-ids#135)) ([fccdf3b](googleapis/java-ids@fccdf3b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…cies to v3.0.3 (#136) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:google-cloud-shared-dependencies](https://github.com/googleapis/java-shared-dependencies) | `3.0.2` -> `3.0.3` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-dependencies/3.0.3/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-dependencies/3.0.3/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-dependencies/3.0.3/compatibility-slim/3.0.2)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-shared-dependencies/3.0.3/confidence-slim/3.0.2)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>googleapis/java-shared-dependencies</summary> ### [`v3.0.3`](https://github.com/googleapis/java-shared-dependencies/blob/HEAD/CHANGELOG.md#​303-httpsgithubcomgoogleapisjava-shared-dependenciescomparev302v303-2022-09-14) [Compare Source](https://github.com/googleapis/java-shared-dependencies/compare/v3.0.2...v3.0.3) ##### Dependencies - Google-cloud-core 2.8.12 ([#​799](https://github.com/googleapis/java-shared-dependencies/issues/799)) ([1b3db8d](https://github.com/googleapis/java-shared-dependencies/commit/1b3db8d1e17c49ebae79fc96164fa9058e1df6e3)) - Moving gson to first-party-dependencies ([#​800](https://github.com/googleapis/java-shared-dependencies/issues/800)) ([a41fcc1](https://github.com/googleapis/java-shared-dependencies/commit/a41fcc11d32e02e5af2837561792e3919f6d4b3f)) - Update dependency com.google.protobuf:protobuf-bom to v3.21.6 ([#​797](https://github.com/googleapis/java-shared-dependencies/issues/797)) ([bc5fdc9](https://github.com/googleapis/java-shared-dependencies/commit/bc5fdc9b3af7973c28f063a9ac156fe2af562814)) - Update gax.version to v2.19.1 ([#​798](https://github.com/googleapis/java-shared-dependencies/issues/798)) ([84e5487](https://github.com/googleapis/java-shared-dependencies/commit/84e5487b2e3dce4bb60badecebde788c3cb702b8)) - Update google.core.version to v2.8.11 ([#​793](https://github.com/googleapis/java-shared-dependencies/issues/793)) ([63c1297](https://github.com/googleapis/java-shared-dependencies/commit/63c129722aa0b821031ff5b4c11004adf7b12044)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-vmmigration). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xOTUuNSIsInVwZGF0ZWRJblZlciI6IjMyLjE5NS41In0=-->
🤖 I have created a release *beep* *boop* --- ## [1.3.2](googleapis/java-vmmigration@v1.3.1...v1.3.2) (2022-09-15) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.3 ([#136](googleapis/java-vmmigration#136)) ([9f6908d](googleapis/java-vmmigration@9f6908d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
* chore: [java] generation with shared deps 3.0.4 * fix for tests Source-Link: https://github.com/googleapis/synthtool/commit/c3ad3cc9d876a3dd897cc511cf5ef921784851ae Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:5643a4e1b729803e67ddceee450e87052527b37cac394bf900b4f8e3d1bb3e9b
http://gerrithub.io/
The text was updated successfully, but these errors were encountered: