node-gh / gh

(DEPRECATED) GitHub CLI made with NodeJS. Use the official https://cli.github.com/ instead.
http://nodegh.io
Other
1.71k stars 217 forks source link

Unable to send pull requests (and peerDependencies errors) #745

Closed julien closed 4 years ago

julien commented 4 years ago

Version number 2.8.1

Describe the bug

After installing the package via

npm i -g gh

A warning is shown

npm WARN marked-terminal@3.2.0 requires a peer of marked@^0.4.0 || ^0.5.0 || ^0.6.0 but none is installed. You must install peer dependencies yourself.

When trying to run gh pr -s SOME_GITHUB_USERNAME an error is thrown

Submitting pull request to SOME_GITHUB_USERNAME
/bin/sh: 1: /tmp/tmp-29718S1GXHRWm3gvv-temp-gh-pr-title.txt: Permission denied
fatal: Could not use your editor to store a custom message
 { Error: Command failed:  "/tmp/tmp-29718S1GXHRWm3gvv-temp-gh-pr-title.txt"
    at checkExecSyncError (child_process.js:629:11)
    at Object.execSync (child_process.js:666:13)
    at Object.execSyncInteractiveStream (/usr/lib/node_modules/gh/lib/exec.js:58:25)
    at Object.openFileInEditor (/usr/lib/node_modules/gh/lib/utils.js:74:16)
    at submit (/usr/lib/node_modules/gh/lib/cmds/pull-request/submit.js:22:25)
    at Object.submitHandler (/usr/lib/node_modules/gh/lib/cmds/pull-request/submit.js:50:26)
  status: 126,
  signal: null,
  output: [ null, null, null ],
  pid: 29755,
  stdout: null,
  stderr: null }

(Obviously SOME_GITHUB_USERNAME is a real username on GitHub, I just didn't want to include it in the bug report for privacy reasons.)

To Reproduce

Please see description above

Expected behavior

I expect this package to work as intended, gh pr -s SOME_GITHUB_USERNAME should submit the pull request to SOME_GITHUB_USERNAME on Github.

Screenshots

image

Additional context

protoEvangelion commented 4 years ago

@julien thanks for reporting this. We added a feature recently to open your editor when you omit --title and --description. We are using your OS's temp directory, but it looks like it would require you to use sudo.

Here are some workarounds until we can solve this better:

In your ~/.gh.json file add "use_editor": false.

OR

sudo gh pr -s SOME_GITHUB_USERNAME

OR

add the title and description

gh pr -s SOME_GITHUB_USERNAME -t "hey" -D "description"

OR make your /tmp dir writable.

We should probably list these options instead of a fatal error.

julien commented 4 years ago

Thanks for the quick reply @protoEvangelion.

I'll add "use_editor": false to ~/.gh.json but I still think this issue should be fixed and not closed, because I can actually write to my /tmp directory, and lots of programs I use on a daily basis do that as well without crashing (it's a standard feature after all).

As an example

const fs = require("fs");
const os = require("os");
const path = require("path");

fs.writeFile(
    path.join(os.tmpdir(), "foo.txt"),
    "This is some text ...\n",
    err => { err && console.error(err) }
);

Works fine on this machine (the file foo.txt is created in /tmp with the content I provided)

The fact that it's JS doesn't really matter, this could be a bash script or a compiled C program and it would have the same result.

protoEvangelion commented 4 years ago

Thanks for the extra info @julien that helps a lot as I can't repro on my machine!

Could you replace https://github.com/node-gh/gh/blob/master/src/utils.ts#L78 with writeFileSync(${os.tmpdir()}-${filename}, msg) and let me know if that works?

If you don't want to build the project you can just find that line and replace it in your node_modules in the utils.js file:

image

julien commented 4 years ago

Hey @protoEvangelion,

Thanks, I think your solution needs some modifications, but it's pretty easy to fix

${os.tmpdir()}-${filename}

Is going to return something like

/tmp-temp-gh-pr-title.txt

Which is not what you want (notice the - being used as the path separator)

I usually use the path module to construct paths, because it can come in handy when dealing with this kind of thing or having to support different OS's

In my case I used:

path.join(os.tmpdir(), fileName);

Just make sure both path and os modules are required because they aren't at the moment.

The second thing that was failing was getting the "editor".

The program tries to get the editor with:

const editor = exec.spawnSync('git', ['config', '--global', 'core.editor']).stdout;

In my case (I try to add the bare minimum configuration)

git config --global core.editor

Doesn't return anything. I usually don't set it and rely on programs to be "smart" enough to read $EDITOR or $VISUAL, and that's what git does too apparently.

But once I configured that, it worked.

I'd still try to read $EDITOR or $VISUAL which should be easy to get with process.env and maybe as a "fallback" try the current solution.

protoEvangelion commented 4 years ago

Awesome suggestions @julien 🎉 Thank you again for the extra details & ideas.

I will give that a try unless you would like to send a PR for it 😄

julien commented 4 years ago

@protoEvangelion I actually have the pull request ready.

One of the tests is failing right now. I saw this but I'm not really sure what to do about it. Is there any way to configure this? I don't really feel like adding my username and password to make tests pass.

julien commented 4 years ago

@protoEvangelion it's ok I figured what was going on.

protoEvangelion commented 4 years ago

@all-contributors please add @julien for bug,code

allcontributors[bot] commented 4 years ago

@protoEvangelion

I couldn't determine any contributions to add, did you specify any contributions? Please make sure to use valid contribution names.

protoEvangelion commented 4 years ago

@all-contributors please add @julien for bug code

allcontributors[bot] commented 4 years ago

@protoEvangelion

I've put up a pull request to add @julien! :tada:

protoEvangelion commented 4 years ago

:tada: This issue has been resolved in version 2.8.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: