-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(git-node): add git node vote
#704
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
=======================================
Coverage 83.38% 83.38%
=======================================
Files 37 37
Lines 4158 4158
=======================================
Hits 3467 3467
Misses 691 691 ☔ View full report in Codecov by Sentry. |
@nodejs/node-core-utils given this PR is relatively large, it would help to have some extra pair(s) of eyes to review it :) |
Since this is specific to tsc, why not make it “git node tsc-vote”? |
It's not specific to the TSC, any other team (or anyone really) is welcome to use it. |
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.
Can you add some docs to https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md?
components/git/vote.js
Outdated
|
||
export const command = 'vote [prid|options]'; | ||
export const describe = | ||
'Manage the current landing session or start a new one for a 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.
This doesn't seem quite 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.
Good catch!
if (this.postComment) { | ||
const { html_url } = await this.req.json(`https://api.github.com/repos/${this.owner}/${this.repo}/issues/${this.prid}/comments`, { | ||
agent: this.req.proxyAgent, | ||
method: 'POST', |
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 this should require additional permissions from the token other than the ones specified by https://github.com/nodejs/node-core-utils/blob/main/README.md#setting-up-github-credentials? So the docs should probably be updated and/or there should be a hint if this returns a permission error code (403?)?
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Co-authored-by: Michaël Zasso <targos@protonmail.com>
This PR adds
git node vote
command, which can be used to participate to a vote using Caritat. The idea of integrating that in ncu is that TSC folks are very likely to already have ncu installed and configured on their local machine, reducing the possible friction for a user to cast a vote, as they don't need to provide their GitHub credentials to make the API calls.Code reviews are welcome, note that we should wait for the transfer of Caritat npm package to the
@node-core
scope to be completed before landing.