-
Notifications
You must be signed in to change notification settings - Fork 6
Fix Slack notification snippet #45
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
We need the snippet to show information about issues and PRs from all repositories in which we typically accept contributions.
ozer550
left a comment
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.
The code seems fine, just one question regarding pagination!
scripts/utils.js
Outdated
| async function hasLabel(name, owner, repo, issueNumber, github, core) { | ||
| let labels = []; | ||
| try { | ||
| const response = await github.rest.issues.listLabelsOnIssue({ |
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.
Just one question here the data received is it paginated? Do we get all the issues in one go?
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.
Do we get all the issues in one go?
We're fetching only one issue here - by the issue number.
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.
As for labels, I will look into it
scripts/utils.js
Outdated
| async function getIssues(assignee, state, owner, repos, github, core) { | ||
| const promises = repos.map(repo => | ||
| github.rest.issues | ||
| .listForRepo({ |
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.
Same for this and getPullRequests function as well? If we are getting a limited amount of issues in one pass then its not whats expected right?
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.
Yes, I will have a look
|
Thank you @ozer550 for good feedback - I pushed updates. It's based on
https://octokit.github.io/rest.js/v20/#pagination Also tested real quickly in my environment and haven't noticed any issues. |
ozer550
left a comment
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.
Changes look good to me!
Summary
Disclosure: I was assisted by copilot - reviewed and adjusted all changes.
Reviewer guidance
Code review should be sufficient. I tested that it works as expected in my test organization and Slack, and after merging we can observe in production.