CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 18
Add check for closed source rationale #96
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the abstractions you've built out here – it makes it way easier to read the default export!
tasks/closedSourceRationaleCheck.ts
Outdated
const missingRationale = await findMissing(repos, api.repos.getContent, org) | ||
|
||
const args = { open: true, owner: org, repo: "0", title: message } | ||
missingRationale.forEach(repo => openIssue(danger.github.utils.createUpdatedIssueWithID, args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will search/edit/create ~100 issues in parallel, which might end up overwhelming the rate limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think we should do instead?
8a0d559
to
efe6584
Compare
Ok fellas, I took another pass at this with some help from @pepopowitz and have pushed up how I think this should go down. @orta brings up a good point re: making a ton of issues and the current implementation does nothing to ensure there isn't already an issue opened for this violation. But I like the direction it's taking and wanted to get something up to keep the momentum going on this sucker. |
const issueURLs = await createIssuesFor(invalidRepos) | ||
|
||
// what should i do with these things?? | ||
console.log(issueURLs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something I should do with these issue URLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashfurrow or @orta any thoughts on what I should do with the return of creating issues? Does it make sense to print these things out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does, only so that you can easily debug later - I commonly leave console logs in the other dangerfiles so that it's easy to read the logs later
Because you're using |
Oh that's great news @orta, did not know that!! ❤️ |
So it looks like we're just blocked on the rate limiting, right? @orta does that reasoning hold? |
Yeah, I think it might even be per-repo on the GitHub app if folks are happy, this will still need hooking up in the peril settings json though |
Ok, did some final polish on this with @ashfurrow and I think this is ready for final review!! ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, let's do it!
Sorry to butt in late, might be worth making that the issue comment give and example reason so peeps can C&P into their READMEs and edit it (to understand the formatting) |
Paired with @ashfurrow on this some today and pushing my WIP for initial feedback.
Fixes artsy/README#131.