Skip to content
This repository has been archived by the owner on Jan 26, 2018. It is now read-only.

Pluggable approval algorithm #32

Open
bradrydzewski opened this issue Jul 12, 2016 · 6 comments
Open

Pluggable approval algorithm #32

bradrydzewski opened this issue Jul 12, 2016 · 6 comments

Comments

@bradrydzewski
Copy link
Member

bradrydzewski commented Jul 12, 2016

I've required a few requests to make alterations to the approval process and include multiple algorithms. I recognize there is not a one-size-fits all approval process. That being said, I am not interested in hosting / supporting / maintaining multiple algorithms in the codebase that would, in all likelihood, only be used by the team that contributed them.

I do believe we can provide some sort of middle ground. I propose the option to delegate approvals to a third party service (ie webhooks). This could be configured at a global level in LGTM using an environment variable:

LGTM_WEBHOOK=http://my.server.com

This would post a JSON payload with the github hook, comments, maintainer file (parsed and raw) and .lgtm configuration details (parsed and raw) to the webhook url. The server would block and wait for the reply, which would include the approval status and relevant metadata.

This would enable pluggable algorithms so that LGTM can be extended without having to host multiple algorithms in the main codebase.

@mspiegel
Copy link

What approval algorithms would users like to see? I know of the current default algorithm in LGTM, and the "org" approval algorithm from #35.

@mspiegel
Copy link

Sorry I didn't read #20. Now I understand this feature request. One argument in favor of multiple approval algorithms in the codebase is that it increases developer participation in the LGTM project. It would be reasonable to include multiple implementations and then in the future delete these implementations if they contain bugs and are not maintained by their developers. I can't tell if @benhutchins intended to submit a pull request with the OWNERS implementation?

@bradrydzewski
Copy link
Member Author

bradrydzewski commented Jul 25, 2016

@mspiegel while I definitely understand this argument, I did not necessarily post the source code with the intention of growing a community around this repository. This is somewhat addressed in the README

LGTM is meant to be extremely simple and focused and is largely considered feature-complete. The author is certainly interested in minor improvements and bug fixes, but is not interested in major enhancements. Feel free to fork the project and extend (and even re-brand) as you see fit.

I understand this might be a bit unconventional since most authors release a repository with the intention of growing a community around it. The difference here is that I have some larger communities (namely drone) that are very demanding of my time and I do not have the time / desire / financial incentive to grow this repository beyond its existing, simple goals.

That being said I'm certainly open to making LGTM more extensible via hooks or some other plugin mechanism.

@bradrydzewski
Copy link
Member Author

What approval algorithms would users like to see?

overall this hasn't been heavily requested, but each time it is requested the algorithm is never the same. Some examples that come to mind are:

  1. the ability to only require an LGTM once per branch, and once that branch is approved to allow all subsequent PRs to get merged
  2. the ability to require the individual that previously edited the code (via git blame) to review and approve changes
  3. to have more complex approval requirements based on subdirectories changed in the project

@mspiegel
Copy link

I see your point and understand your concerns. @jonbodner and I are going to fork this repository and maintain additional features in the fork. GitHub disables some features (most notably searching) from repositories that are forks of another repository. So we're going to create a new GitHub repository and rebrand the project. We'll make sure to include a prominent note in the README file to this repository in order to have proper attribution. The license will remain Apache 2.

Does that sound good to you?

@bootstraponline
Copy link

I think pluggable approval makes sense. This would allow GitHub teams support so we don't have to hard code the maintainers in every repo.

jonbodner pushed a commit to capitalone/checks-out that referenced this issue Nov 21, 2017
Please read the README and the CHANGELOG for a list of features
developed in this fork. Initial discussion of the fork can be
found at lgtmco/lgtm#32.

New features have been developed by: @mspiegel, @jonbodner, @jonathana
jonbodner pushed a commit to capitalone/checks-out that referenced this issue Nov 21, 2017
Please read the README and the CHANGELOG for a list of features
developed in this fork. Initial discussion of the fork can be
found at lgtmco/lgtm#32.

New features have been developed by: @mspiegel, @jonbodner, @jonathana
jonbodner pushed a commit to capitalone/checks-out that referenced this issue Nov 21, 2017
Please read the README and the CHANGELOG for a list of features
developed in this fork. Initial discussion of the fork can be
found at lgtmco/lgtm#32.

New features have been developed by: @mspiegel, @jonbodner, @jonathana
mspiegel added a commit to capitalone/checks-out that referenced this issue Dec 4, 2017
Please read the README and the CHANGELOG for a list of features
developed in this fork. Initial discussion of the fork can be
found at lgtmco/lgtm#32.

New features have been developed by: @mspiegel, @jonbodner, @jonathana
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

3 participants