justinabrahms / imhotep

A static-analysis bot for Github
MIT License
223 stars 42 forks source link

Initial Stash support for PRs #74

Closed jbeluch closed 2 years ago

jbeluch commented 9 years ago

Not quite ready for merge (only PRs are working), but wanted to see if this is something you'd like contributed.

If so, I figure you'll have some ideas about how you want to handle the extra arguments necessary for stash. Currently just passing around kwargs everywhere but obviously that's not optimal.

Since stash is always (I think) hosted privately, we need a stash_hostname. Also, stash is organized by project then by repo, so there is a project_name argument as well.

Lastly, I changed githubusername/password to drop the github part so it can be reused.

justinabrahms commented 9 years ago

I am very into this contribution. I'll spend some time this evening reviewing, but wanted to say THANK YOU!

justinabrahms commented 9 years ago

The initial looks pretty good.

I'd call your 'stash_url' a 'base_url' which will be necessary for things like github enterprise anyway.

Security note is well taken. See #75.

We'll need to introduce a dispatcher for "Oh, this is stash. Use this reporter, etc".

Solid start though. Thanks! :)

jbeluch commented 9 years ago

The security thing was a copy paste from elsewhere in your repo :)

As far as the dispatcher goes, how will we know which service is requested? The URLs won't necessarily give away that info.

Separate CLI flag/config option? positional arg? nested config file under a key 'stash' or 'github'?

justinabrahms commented 9 years ago

I'd just add another flag. --service=github/stash with github as the default.

justinabrahms commented 2 years ago

Feel free to open when you're excited about it again.