Skip to content
This repository has been archived by the owner on Oct 7, 2023. It is now read-only.

enabling reviewable #13

Open
philips opened this issue Jul 16, 2019 · 6 comments
Open

enabling reviewable #13

philips opened this issue Jul 16, 2019 · 6 comments

Comments

@philips
Copy link

philips commented Jul 16, 2019

@tbg put in a request to enable reviewable.io on the etcd-io organization.

@tbg - can you provide a bit of information on what you hope to use this for?

@philips
Copy link
Author

philips commented Jul 16, 2019

cc @etcd-io/maintainers-etcd

@tbg
Copy link

tbg commented Jul 16, 2019

@philips thanks for getting back to this so quickly. We use Reviewable extensible (and have for years) over in https://github.com/cockroachdb/cockroach/ and have found it to improve our review workflow immensely for nontrivial PRs. It's very well maintained and with responsive support/bug fixes over the years.

I've recently been posting more complicated PRs (etcd-io/etcd#10889, etcd-io/etcd#10892) against etcd/raft that go through multiple revisions and the improved comment/diff tracking that Reviewable offers is worth it in these cases.

With full integration, Reviewable adds a comment with a link to each newly opened PR, but I did try to use it "just for my PR" and it almost just worked - apparently the org-level auth is needed to be able to post the comments back from Reviewable into a Github comment:

https://gitter.im/Reviewable/Reviewable?at=5d2c5a1f202bb93f00a88006

My hope was that there's a middle ground where folks that want to use Reviewable on their PR can do so, without getting into a bigger discussion on whether to switch the review workflow over (I'd do it without batting an eye for any repo I own though).

Reviewable does pick up any Github reviews that were left and puts them in the right places. It does not, however, leave its reviews as line-anchored Github reviews - it uses summary comments as can be seen in cockroachdb/cockroach#38084 (review). At the end of the day what ends up happening in my experience is that simple reviews stay off Reviewable and substantial ones are held there.

@spzala
Copy link
Member

spzala commented Jul 16, 2019

@tbg with due respect to your proposal and agreeing that reviewable has advantages for larger PRs, while we can continue discussions and latest findings, I just wanted to mentioned this discussion in the Kubernetes. It was tried and later disabled in all Kubernetes repos due to its poor usage and also it seems like it can cost CNCF money.

  1. https://groups.google.com/forum/#!topic/kubernetes-dev/0RhWYvxW2XI - in this thread, Tim Hockins has mentioned I'm considering turning off reviewable.k8s.io. It gets very low usage and costs CNCF money to run.
  2. disable reviewable for kubernetes repos that don't use it kubernetes/community#1881 - the issue related to disabling reviewable in Kubernertes repos that don't use it. Per Tim's comment in the issue at the end, all of the Kubernetes repos were disabled.
  3. Disable the "Reviewable" button and review access enabled for kubernetes/website kubernetes/website#7552 - an issue related to above, where along with poor usage some problems with github automation with reviewable is also mentioned. We don't have much automation currently but in future we might.

Thanks!

@tbg
Copy link

tbg commented Jul 16, 2019

@spzala thanks for your comment. I don't think I want to push for the use of reviewable and there doesn't seem to be any other champion. It would've been nice had it worked without repo integration, but it also seems that this might become possible in the future.

@philips, mind closing this?

@philips
Copy link
Author

philips commented Jul 16, 2019

@tbg I will let this sit open for another week in case other etcd maintainers have opinions.

@hexfusion
Copy link

gRPC team has used reviewable this we should reach out them and get feedback.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants