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

"Permission denied" when trying to send pull request without providing title or description #717

Closed dantewang closed 4 years ago

dantewang commented 4 years ago

Version number 2.3.3

Describe the bug

My gh is installed using sudo gh i -g gh while I always use it without root permission. gh is installed in /usr/lib/node_modules/gh.

After updating to 2.3.3, when trying to send any pull request using gh without providing title (-t) or description (-D), the operation fails because it tries to access a file in "/usr/lib".

For example, if -D is not supplied:

fatal: Could not use your editor to store a custom message
 Error: EACCES: permission denied, open '/usr/lib/node_modules/gh/lib/temp-gh-pr-body.md'
    at Object.openSync (fs.js:440:3)
    at Object.writeFileSync (fs.js:1265:35)
    at Object.openFileInEditor (/usr/lib/node_modules/gh/lib/utils.js:47:14)
    at submit (/usr/lib/node_modules/gh/lib/cmds/pull-request.js:674:31)
    at _submitHandler (/usr/lib/node_modules/gh/lib/cmds/pull-request.js:828:26)
    at async main (/usr/lib/node_modules/gh/lib/cmds/pull-request.js:191:13)
    at async Object.run (/usr/lib/node_modules/gh/lib/cmds/pull-request.js:130:9)
    at async /usr/lib/node_modules/gh/lib/cmd.js:174:13 {
  errno: -13,
  syscall: 'open',
  code: 'EACCES',
  path: '/usr/lib/node_modules/gh/lib/temp-gh-pr-body.md'
}

To Reproduce Steps to reproduce the behavior:

  1. Run Cmd 'gh pr -s username'
  2. See error

Expected behavior

Before 2.3.3, if -t is not supplied, the commit message of the last commit of the current branch will be used as pull request title; if -D is not supplied, the description will be empty. No errors should be thrown even if gh is installed by sudo npm i -g gh but executed without root permission.

protoEvangelion commented 4 years ago

@dantewang we just released a feature where if you leave --title or --description empty it will open your default git editor where you can supply the message (which is really nice for markdown).

The reason it is failing is because we are saving a temp file and then opening that in your editor.

Do you have any ideas on ways to get around that without sudo?

andreldm commented 4 years ago

@dantewang we just released a feature where if you leave --title or --description empty it will open your default git editor where you can supply the message (which is really nice for markdown).

Is there an option to disable this (i.e. keep description empty)?

protoEvangelion commented 4 years ago

@andreldm there is no option currently to keep description empty. However, if you close your editor without adding text, it will keep the description empty.

dantewang commented 4 years ago

@protoEvangelion I think the temp files should be put in either /tmp or some place under ~/.

dantewang commented 4 years ago

@protoEvangelion Also, it will be annoying if I have to close my text editor twice when sending a pull request. I would prefer:

Also, does PR title support markdown? If not, what's the point to provide this feature for title?

protoEvangelion commented 4 years ago

@protoEvangelion I think the temp files should be put in either /tmp or some place under ~/.

What system are you on? Can you check if you can read/write to /tmp? I don't have any issues placing it there, other than double checking that /tmp dir will work across OSs.

Regarding your suggestions, I think they are reasonable. The work was done in reference to a long standing issue: https://github.com/node-gh/gh/issues/34

What would you propose the the functionality for creating new issues to be? We do not compute the title for that but fallback to the same functionality for PRs.

Also, does PR title support markdown? If not, what's the point to provide this feature for title?

Convenience. Fair point however that there is not much benefit to prompting for a title as there is for opening the editor for markdown.

andreldm commented 4 years ago

@protoEvangelion I think the temp files should be put in either /tmp or some place under ~/.

What system are you on? Can you check if you can read/write to /tmp? I don't have any issues placing it there, other than double checking that /tmp dir will work across OSs.

You can use mktemp which works on Linux (e.g. /tmp/tmp.ku6Z6xMgf7) and macOS (e.g. /var/folders/pf/f5htxln96_18_52b7h72qrn40000gq/T/tmp.pyDaStfI).

dantewang commented 4 years ago

@protoEvangelion I use Manjaro and I think it should work.

My suggestion is based on my own working scenario -- I often send pulls just to run my changes through CI, which doesn't need description. So I'd love to see the old behavior unchanged, with some kind of new ways to trigger the new feature. Other people may prefer the current behavior as default. I think this topic can be moved to another issue.

protoEvangelion commented 4 years ago

Regarding the need to use sudo, I don't see a way around this unless you know of a directory that has write permissions for groups and others.

Both of the common places to store temp files on mac at least do not have write permissions across the board:

image

Regarding the CI use case, it sounds like we just need a simple config setting to turn this feature on and off.

protoEvangelion commented 4 years ago

@dantewang can you create a temp file with mktemp without sudo?

protoEvangelion commented 4 years ago

Just released 2.5.0 where you can add "use_editor": false to turn this feature off.

gamerson commented 4 years ago

I'm running into this error as well. How do you reinstall an older version? Or workaround this issue?

protoEvangelion commented 4 years ago

@gamerson it depends on your context. If you are running in CI, then add the option in your ~/.gh.json as mentioned above. Otherwise you can install an npm package by version number: npm i -g gh@2.4.0

gamerson commented 4 years ago

Thanks Ryan, I'll downgrade until the mentioned error can be worked out.

On Tue, Nov 5, 2019 at 7:54 PM Ryan Garant notifications@github.com wrote:

@gamerson https://github.com/gamerson it depends on your context. If you are running in CI, then add the option in your ~/.gh.json as mentioned above. Otherwise you can install an npm package by version number: npm i -g gh@2.4.0

ā€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/node-gh/gh/issues/717?email_source=notifications&email_token=AAERKFLHTFQU7AR2ODO25HLQSIPXDA5CNFSM4JIQPZO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDE7FZA#issuecomment-550105828, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAERKFMILHLIGGWPADXMH2LQSIPXDANCNFSM4JIQPZOQ .

-- Greg Amerson Liferay Developer Tools Liferay, Inc. www.liferay.com

dantewang commented 4 years ago

@protoEvangelion

I can create a temp dir using mktemp without sudo.

[dante@dante-pc ~]$ mktemp -d /tmp/my-gh-temp-XXXXXXX
/tmp/my-gh-temp-TWATSEH

I believe every modern OS provides a place for application to store temp files without the need of privilege elevation (e.g. sudo in Linux/Mac or UAC dialog in Windows).

For Linux, /tmp is the place to go. This is defined in a Linux standard which can be trusted. See https://superuser.com/questions/332610/where-is-the-temporary-directory-in-linux

For Windows, the location varies in different versions, but there are environment variables to query for the location. See https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha#remarks

For Mac, the temp dir seems to be the ones you tried to use. If so, they SHOULD be writable.

Even if the temp dir approach does not work, it's acceptable to use the user's home dir. gh already saves its config file there.

And just to clarify my use case a bit: 50% of my pull request sent does NOT need title/description to be supplied, but the other 50% does. And some in the latter ones does need a complicated markdown description which can only be done via the editor. This (opening editor) is a nice feature that I'll of course enjoy, but I want it to be on-demand.

protoEvangelion commented 4 years ago

@dantewang @gamerson @andreldm could you please try v 2.5.1?

A simple test would be opening a new issue (this should open your editor to edit the issue body):

gh is --new -t hey

This should be OS agnostic, and not cause permission issues anymore :) My machine doesn't have permission issues so I am relying on you all to test šŸ™ˆ

gamerson commented 4 years ago

2.5.1 is all fixed for me!

dantewang commented 4 years ago

Cool, it works for me!

protoEvangelion commented 4 years ago

@dantewang thanks for clarifying:

And just to clarify my use case a bit: 50% of my pull request sent does NOT need title/description to be supplied, but the other 50% does. And some in the latter ones does need a complicated markdown description which can only be done via the editor. This (opening editor) is a nice feature that I'll of course enjoy, but I want it to be on-demand.

Regarding the "on demand" aspect, the "use_editor": false setting should resolve the use cases of being in the context of a CI or if you just want to feature off completely. However if you want to keep the setting on, currently this will behave like git interactive cmds.

Take git rebase -i for instance. If you select a couple of commits to rename, it will open each commit individually in your git default editor even though often times it is a simple couple character change. But it also works for more complex renaming where you have a long body for a commit message. The way that git solves both use cases, seems to map to our discussion 1 to 1 in that they use your default editor for simple and complex editing tasks.