-
-
Notifications
You must be signed in to change notification settings - Fork 843
Add context param to check-team-membership.js and update calls #8277
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
Add context param to check-team-membership.js and update calls #8277
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. |
|
Review ETA: 6PM 08/22/25 |
Thanks @praytoo ! A couple of comments:
Thank you! |
|
@DDVVPP hello thank you for your feedback i've gone ahead and checked off the action items on the list like you suggested...and can you see if the changes are showing up on your end now? i believe i updated the pr to reflect the changes. thanks again! |
|
Review ETA: 08/22/25 |
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.
Thank you for your hard work on this issue @praytoo ! I have some changes to request, but afterwards I can approve your issue.
Things Done Well
- PR description accurately describes changes and why they were made
- Code does not trigger any QL alerts
Changes to Make
- I agree with DDVVPP in that I can't seem to see all the changes that you made. Only
check-team-member.jsis showing up in the "Files changed" tab on the pull request. Do you mind double checking again if you pushed all of your changes forverify-pr.jsandissue-trigger.yml? - In
check-team-membership.js, I noticed you removed some of the lines and parameters in the commented block in the beginning. I think the instructions in the issue are to only modify/add the requested lines and leave the rest as-is. - In
check-team-membership.js, I think part of line 5 may have been accidentally deleted, and should be "Need read:org permission to use this function. Lack of permission will result in a 403 error."
|
Hi @praytoo! |
t-will-gillis
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.
Hey @praytoo This looks great! The branches are correct, you are linked to the original issue, you are describing what was done and why, you checked for CodeQL alerts, and you note that there are no visual changes. The context object is added to the files currently using check-team-membership.js and you made the edits requested by the issue.
One change is needed, and this isn't something you missed- it is because I misled you when I wrote the original issue.
- In the file
check-team-membership.js, please replace the line for "org" and use instead:org: context.repo.owner,
I realized that context.repo.org would cause an error because the context.repo object only has an owner property, so org would cause an error. The replacement should work. My bad.
Except for the change, I tested this in my repo and the workflows appear to be working correctly. Thanks for working on this!
|
hello @caz002 @DDVVPP @t-will-gillis thank you all for your valuable feedback i've gone ahead and applied all of those corrections and requested a re-review. |
caz002
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.
Thank you for making all of the changes @praytoo ! The pull request correctly modifies the files requested, and the description clearly describes the changes made. I approve this pull request.
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.
Thanks for all the changes @praytoo !
I'm approving these changes, but I do have one more question about a change in the check-team-membership.js file. The items in lines 7, 10, and 12 are missing the dashes, and I'm just curious about why those were elimated!
Thanks again!
t-will-gillis
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.
Hi @praytoo Thank you for making the change- everything looks great!
I am Approving also, and I will merge this.
@DDVVPP I saw your note about the comments and removing the '-'. I can go either way about the dashes, however, I am also thinking that the comments after * @param {String} team ... are extraneous, and using JSDoc there should only be a simple, concise description of the function as the first line (before * @param {octokit} ...
@praytoo @DDVVPP @caz002, any thoughts about this? If needed, we can catch this in a new issue.
|
I deleted the extraneous lines before but was corrected by @caz002 to keep those in about the "-" I didn't think it mattered because the "*" were there to bullet point the lines anyway @DDVVPP lmk what you think @t-will-gillis thanks everyone |
Fixes #8201
What changes did you make?
contextparameter to check-team-membership.jsWhy did you make the changes (we will use this info to test)?
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)
No visual changes to the website