Skip to content
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 warning message for endpoint starting with / in gh api <endpoint> #6476

Closed
wants to merge 2 commits into from

Conversation

dojutsu-user
Copy link
Contributor

Fixes #6415

Description
Added a warning message for the user whenever the endpoint is starting with a slash.

Before

❯ GH_DEBUG=true ./bin/gh api users/repos
* Request at 2022-10-21 01:16:33.038292 +0530 IST m=+0.030087293
* Request to https://api.github.com/users/repos

❯ GH_DEBUG=true ./bin/gh api /users/repos
* Request at 2022-10-21 01:16:49.221381 +0530 IST m=+0.023171918
* Request to https://api.github.com/users/repos

After

❯ GH_DEBUG=true ./bin/gh api users/repos
* Request at 2022-10-21 01:14:42.116196 +0530 IST m=+0.026859043
* Request to https://api.github.com/users/repos

❯ GH_DEBUG=true ./bin/gh api /users/repos
! warning: '/users/repos' An absolute window-styled path was passed which is likely to fail. Try dropping or escaping the starting slash in the endpoint if it fails.
* Request at 2022-10-21 01:15:06.412425 +0530 IST m=+0.026561084
* Request to https://api.github.com/users/repos

@dojutsu-user dojutsu-user marked this pull request as ready for review October 20, 2022 19:57
@dojutsu-user dojutsu-user requested a review from a team as a code owner October 20, 2022 19:57
@dojutsu-user dojutsu-user requested review from samcoe and removed request for a team October 20, 2022 19:57
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Oct 20, 2022
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Oct 20, 2022
@dojutsu-user
Copy link
Contributor Author

Hi @samcoe,
Please add the hacktoberfest-accepted label in this PR.

@dojutsu-user
Copy link
Contributor Author

@samcoe Can you please review this PR.
cc: @mislav

@@ -175,6 +175,11 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command
opts.RequestPath = args[0]
opts.RequestMethodPassed = c.Flags().Changed("method")

if strings.HasPrefix(opts.RequestPath, "/") {
warningIcon := opts.IO.ColorScheme().WarningIcon()
fmt.Fprintf(opts.IO.ErrOut, "%s warning: '%s' An absolute window-styled path was passed which is likely to fail. Try dropping or escaping the leading slash in the endpoint if it fails. \n", warningIcon, opts.RequestPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I think you might have misunderstood the solution I've proposed in the issue.

The problem isn't that some users pass in a path argument with a leading slash. That style of usage is perfectly valid and we shouldn't warn against it.

The problem is that the mingw environment converts those arguments with a leading slash into an absolute, Windows-style file path before invoking gh, which then breaks the gh api command. When that happens, we should error out explaining to the user what happened.

So, what we can do is:

  1. Check whether the argument is a Windows-style absolute path. This can be done with runtime.GOOS == "windows" && filepath.IsAbs(opts.RequestPath)
  2. Error out and explain to the user that they shouldn't use leading slash in their environment.

Think you could do it?

@samcoe samcoe self-assigned this Nov 2, 2022
@samcoe
Copy link
Contributor

samcoe commented Jan 4, 2023

Going to close this as stale as contributor has not responded to requested changes.

@samcoe samcoe closed this Jan 4, 2023
The GitHub CLI automation moved this from Needs review 🤔 to Done 💤 Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

gh api fails due to absolute path expansion in git bash
4 participants