search

artsy / peril-settings

Artsy's peril settings
MIT License
31 stars 18 forks source link

@ashfurrow => Adds rule to generate nice deploy summary #FF #133

Closed mzikherman closed 5 years ago

mzikherman commented 5 years ago

This rather aggressively adds a peril rule simultaneously with the corresponding RFC https://github.com/artsy/README/issues/238 . I forgot about our RFC process when deciding to work on this for Future Fursday 😅 .

I was able to include this rule in a sample project, and run it as a danger rule. This seemed to work and produce the desired output on deploy PR's, while skipping non-deploy PR's.

So, assuming the RFC is agreed on, this is that implementation.

I'll add some tests as well.

ashfurrow commented 5 years ago

Looks great so far! My only feedback is that I'd be concerned with GitHub API rate limiting (5000 requests / hour for the whole of Artsy's Peril install). @zephraph mentioned that he's built something similar for @auto-it using GitHub's GraphQL API, to coalesce the many search requests to one. Justin, could you link to that code for us?

mzikherman commented 5 years ago

Yea, there's potentially a bunch of requests, depending on number of commits. Each commit will trigger a request to find the relevant PR, and then each PR will trigger a request to fetch its own details.

A graph-y version that would count as 1 API request would be great!

Sidenote- is that accessible already in Peril/Danger? I think danger.github.api is the REST API wrapper, will have to do some digging to find (or add) the GraphQL API wrapper.

Either way that seems like a worthy modification to this.

ashfurrow commented 5 years ago

Hmm yeah, good point. I found this one solution. It's not great (ideally we could get back an octokit/graphql.js instance) but it should work 👍

This seems like something we'd like Peril to support, I'll open an issue.

mzikherman commented 5 years ago

Sweet! I'll continue hacking on this.

mzikherman commented 5 years ago

@ashfurrow on some further reflection, I wonder if (assuming the RFC passes), we could give it a shot w/ the REST implementation (before switching to GraphQL). This should only happen on PR's to release, and it seems unlikely we'll run into the 5k/hour limit. Maybe we could monitor and if we notice an issue we can disable the rule and make that switch?

(I'm still going to write some tests here either way, plus the RFC is still open, so no rush/not mergeable yet).

ashfurrow commented 5 years ago

@mzikherman yeah, that sounds good 👍 That would also relieve my stress of getting that feature in to Danger before I leave for vacation Wednesday 😅

mzikherman commented 5 years ago

Updated with a spec.

mzikherman commented 5 years ago

Ok, going for this.