Skip to content
This repository has been archived by the owner on Nov 4, 2022. It is now read-only.

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

Closed
julien opened this issue Nov 21, 2019 · 12 comments · Fixed by #748
Closed

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

julien opened this issue Nov 21, 2019 · 12 comments · Fixed by #748

Comments

@julien
Copy link
Contributor

julien commented Nov 21, 2019

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

  • node version: v10.16.3
  • npm version: 6.9.0
  • Also tried with yarn v1.19.1
  • With previous versions (and on the same hardware) this was working fine.
@julien julien added the bug label Nov 21, 2019
@protoEvangelion
Copy link
Member

protoEvangelion commented Nov 22, 2019

@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
Copy link
Contributor Author

julien commented Nov 23, 2019

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
Copy link
Member

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
Copy link
Contributor Author

julien commented Nov 23, 2019

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
Copy link
Member

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
Copy link
Contributor Author

julien commented Nov 23, 2019

@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 pushed a commit to julien/gh that referenced this issue Nov 23, 2019
To determine which editor to use for commit messages
look if $EDITOR or $VISUAL are set in the environment
before checking the git config. If no editor is found
throw an error. Fixes node-gh#745
@julien
Copy link
Contributor Author

julien commented Nov 23, 2019

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

@protoEvangelion
Copy link
Member

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

@allcontributors
Copy link
Contributor

@protoEvangelion

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

@protoEvangelion
Copy link
Member

@all-contributors please add @julien for bug code

@allcontributors
Copy link
Contributor

@protoEvangelion

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

protoEvangelion pushed a commit that referenced this issue Nov 25, 2019
To determine which editor to use for commit messages
look if $EDITOR or $VISUAL are set in the environment
before checking the git config. If no editor is found
throw an error. Fixes #745
protoEvangelion pushed a commit that referenced this issue Nov 25, 2019
## [2.8.3](v2.8.2...v2.8.3) (2019-11-25)

### Bug Fixes

* apply suggested pr review changes ([81624e7](81624e7))
* check for editor in environment variables ([192e142](192e142)), closes [#745](#745)
@protoEvangelion
Copy link
Member

🎉 This issue has been resolved in version 2.8.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants