-
Notifications
You must be signed in to change notification settings - Fork 9
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
Self hosted version #85
Comments
Yeah this can definitely be self hosted. You'll just have to expose an |
Let me know if you want to pair on this at all. I think it would be great to add documentation to the README on how to self host. |
I think 2 things will be a roadblock:
I'd suggest adding |
And I'll love to pair on this! Let me know if you want me to send a PR for above. |
Yeah those are good points. It seems fairly trivial to abstract logging/raven out, and expose a way to setup GitHub Enterprise URL's. A PR for those would be great :) |
Cool! I'll get going on this then... |
hey @mrchief, I just merged in your pull requests and deployed them. Everything seems to be working great! I think the next step would just be documenting how to self host. A rough outline:
|
Thanks and sure thing! I'm in the process of deploying it internally. Once I'm done with that, I'll send a doc update. |
@all-contributors please add @mrchief for infrastructure, tests and code |
I've put up a pull request to add @mrchief! 🎉 |
Hey @ewolfe! Update on deployment to a private repo - I think we need to segregate the part that does the validation from the part that handles the web server. Something like prlint-core which:
Step 1 this could be baked in as default with an optional way left to the caller to retrieve it however they want. This would go well with any internal infosec requirements. This way, the user can host it however their org prefers (Dedicated servers, lambdas etc.) without having to fork the repo. It's easier to stay updated (and in sync with the root repo) as it's a matter of upgrading the package version as opposed to having to maintain a fork. If this sounds good to you, I can send in a PR. |
Yeah that sounds like a really good idea. I'll definitely accept a pull request for that! |
Hey @ewolfe - do you know why we have 2 different ways of figuring out the pull request's status URL? https://github.com/ewolfe/prlint/blob/master/src/app.js#L116 and https://github.com/ewolfe/prlint/blob/master/src/app.js#L140 |
In fact, it appears that #L116 shouldn't work since it'd look like The logic in #L140 will produce a similar URL with the |
Oh right, I think that was to cover the case where the pull request was coming from a forked version of the repo. It's been a while though so I could be wrong. |
I've been playing with this on a couple repos in my org. I like the extensibility of using regex for inspecting things, but need to be able to inspect commits too. There are a couple of other tools which are doing similar things, but this seems the most straightforward to get our teams to adopt. I'd like to make this easier to self host in a docker/kubernetes environment. If that sounds interesting I can contribute back my changes. For now I'm going to hack in the changes I need and build an image, but when this PR merges I can try out the probot based version. |
@moustafab have you worked out a way to deploy PRLint into ECS or as a Lambda? |
Sounds great @moustafab! I think we wanted to move it to its own org and that's where things fell in limbo.
I remember adding commit linting into it as well (internally if course). I can share more details later. @echernyavskiy I'm using this as a lambda internally. Probot apps can be deployed as lambdas as stated in their docs. There are some quirks though and I can share my recipe if that helps. Both I & @ewolfe have not been able to find time to move this forward so any help is much appreciated! |
I'd appreciate it, thanks. |
Is it possible to deploy this to a self-hosted service? The source code kind of assumes public GitHub for URLs and stuff so it doesn't seem possible at the moment but it'd be cool if it did support that.
The text was updated successfully, but these errors were encountered: