From 5fad092b9e7f4b847a77dffe883d71e6310df416 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 5 Sep 2021 01:01:58 -0700 Subject: [PATCH 001/167] Refactor Windows Installer setup Resolves #703 along with several other issues: * Build an x64 MSI for an x64 executable. This means the binary is installed to C:\Program Files, by default, rather than C:\Program Files (x86) without the ability to redirect it to 64-bit locations. * Environment change to PATH is not system-wide, which for a per-machine install it should be so all users who can access the executable have it in their PATH. * Environment change to PATH is not cleaned up when uninstalled. * RTF conversion of LICENSE was difficult to read. A simple conversion script is checked in to facilitate regenerating RTF from root LICENSE. --- .github/workflows/releases.yml | 14 +++----- build/windows/ConvertTo-Rtf.ps1 | 24 ++++++++++++++ build/windows/LICENSE.rtf | 23 +++++++++++++ build/windows/gh.wixproj | 35 ++++++++++++++++++++ build/windows/gh.wxs | 58 +++++++++++++++++++++++++++++++++ wix.json | 32 ------------------ 6 files changed, 144 insertions(+), 42 deletions(-) create mode 100644 build/windows/ConvertTo-Rtf.ps1 create mode 100644 build/windows/LICENSE.rtf create mode 100644 build/windows/gh.wixproj create mode 100644 build/windows/gh.wxs delete mode 100644 wix.json diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 79e150113..cbba42cb9 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -123,23 +123,17 @@ jobs: unzip -o *.zip && rm -v *.zip env: GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}} - - name: Install go-msi - run: choco install -y "go-msi" - name: Prepare PATH - shell: bash - run: | - echo "$WIX\\bin" >> $GITHUB_PATH - echo "C:\\Program Files\\go-msi" >> $GITHUB_PATH + uses: microsoft/setup-msbuild@v1.0.3 - name: Build MSI id: buildmsi shell: bash env: ZIP_FILE: ${{ steps.download_exe.outputs.zip }} run: | - mkdir -p build - msi="$(basename "$ZIP_FILE" ".zip").msi" - printf "::set-output name=msi::%s\n" "$msi" - go-msi make --msi "$PWD/$msi" --out "$PWD/build" --version "${GITHUB_REF#refs/tags/}" + name="$(basename "$ZIP_FILE" ".zip")" + printf "::set-output name=msi::%s\n" "$name.msi" + msbuild .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/}" - name: Obtain signing cert id: obtain_cert env: diff --git a/build/windows/ConvertTo-Rtf.ps1 b/build/windows/ConvertTo-Rtf.ps1 new file mode 100644 index 000000000..401951cc8 --- /dev/null +++ b/build/windows/ConvertTo-Rtf.ps1 @@ -0,0 +1,24 @@ +[CmdletBinding()] +param ( + [Parameter(Mandatory=$true, Position=0)] + [string] $Path, + + [Parameter(Mandatory=$true, Position=1)] + [string] $OutFile, + + [Parameter()] + [ValidateNotNullOrEmpty()] + [string] $FontFamily = 'Arial' +) + +$rtf = "{\rtf1\ansi\deff0{\fonttbl{\f0\fcharset0 $FontFamily;}}\pard\sa200\sl200\slmult1\fs20`n" +foreach ($line in (Get-Content $Path)) { + if (!$line) { + $rtf += "\par`n" + } else { + $rtf += "$line`n" + } +} +$rtf += '}' + +$rtf | Set-Content $OutFile \ No newline at end of file diff --git a/build/windows/LICENSE.rtf b/build/windows/LICENSE.rtf new file mode 100644 index 000000000..185063f24 --- /dev/null +++ b/build/windows/LICENSE.rtf @@ -0,0 +1,23 @@ +{\rtf1\ansi\deff0{\fonttbl{\f0\fcharset0 Arial;}}\pard\sa200\sl200\slmult1\fs20 +MIT License +\par +Copyright (c) 2019 GitHub Inc. +\par +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: +\par +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. +\par +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. +} diff --git a/build/windows/gh.wixproj b/build/windows/gh.wixproj new file mode 100644 index 000000000..56a87fb66 --- /dev/null +++ b/build/windows/gh.wixproj @@ -0,0 +1,35 @@ + + + + Debug + x64 + 0.1.0 + $(MSBuildProjectName) + package + $(MSBuildProjectDirectory)\..\.. + $(RepoPath)\bin\$(Platform)\$(Configuration)\ + $(RepoPath)\bin\obj\$(Platform)\$(Configuration)\ + + $(DefineConstants); + ProductVersion=$(ProductVersion); + + false + $(MSBuildExtensionsPath)\Microsoft\WiX\v3.x\Wix.targets + + + + + + + + + + + + + + + + + + diff --git a/build/windows/gh.wxs b/build/windows/gh.wxs new file mode 100644 index 000000000..7d2f967d5 --- /dev/null +++ b/build/windows/gh.wxs @@ -0,0 +1,58 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/wix.json b/wix.json deleted file mode 100644 index 6b2356ae3..000000000 --- a/wix.json +++ /dev/null @@ -1,32 +0,0 @@ -{ - "product": "GitHub CLI", - "company": "GitHub, Inc.", - "license": "LICENSE", - "upgrade-code": "7c0a5736-5b8e-4176-b350-613fa2d8a1b3", - "files": { - "guid": "6e6dcb19-3cf6-46d1-ac56-c6fb39485c9d", - "items": [ - "bin/gh.exe" - ] - }, - "env": { - "guid": "94faac3d-4478-431c-8497-fba55dcfb249", - "vars": [ - { - "name": "PATH", - "value": "[INSTALLDIR]", - "permanent": "yes", - "system": "no", - "action": "set", - "part": "last" - } - ] - }, - "shortcuts": {}, - "choco": { - "description": "Use GitHub from the CLI", - "project-url": "https://github.com/cli/cli", - "tags": "github cli git", - "license-url": "https://github.com/cli/cli/blob/trunk/LICENSE" - } -} From cb599af1a33789e2d6be93b4be6a93c428e4b4b0 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 5 Sep 2021 09:15:36 -0700 Subject: [PATCH 002/167] Make sure correct step output is set Also simplifies directories for an always-release binary. --- .github/workflows/releases.yml | 2 +- build/windows/gh.wixproj | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index cbba42cb9..3acd23acd 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -133,7 +133,7 @@ jobs: run: | name="$(basename "$ZIP_FILE" ".zip")" printf "::set-output name=msi::%s\n" "$name.msi" - msbuild .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/}" + msbuild .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputPath="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/}" - name: Obtain signing cert id: obtain_cert env: diff --git a/build/windows/gh.wixproj b/build/windows/gh.wixproj index 56a87fb66..db5f1b0c7 100644 --- a/build/windows/gh.wixproj +++ b/build/windows/gh.wixproj @@ -1,14 +1,14 @@ - Debug + Release x64 0.1.0 $(MSBuildProjectName) package - $(MSBuildProjectDirectory)\..\.. - $(RepoPath)\bin\$(Platform)\$(Configuration)\ - $(RepoPath)\bin\obj\$(Platform)\$(Configuration)\ + $([MSBuild]::NormalizeDirectory($(MSBuildProjectDirectory)\..\..)) + $(RepoPath)bin\$(Platform)\ + $(RepoPath)bin\obj\$(Platform)\ $(DefineConstants); ProductVersion=$(ProductVersion); @@ -31,5 +31,9 @@ + + + + From 2fa3de9ba49ca28223a5a01026ce81d8c87c42d6 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Tue, 7 Sep 2021 23:02:59 -0700 Subject: [PATCH 003/167] Resolve PR feedback --- .github/workflows/releases.yml | 3 +-- build/windows/gh.wixproj | 1 + build/windows/gh.wxs | 30 +++++++++++++++++++++++++----- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 3acd23acd..63a3891e6 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -132,8 +132,7 @@ jobs: ZIP_FILE: ${{ steps.download_exe.outputs.zip }} run: | name="$(basename "$ZIP_FILE" ".zip")" - printf "::set-output name=msi::%s\n" "$name.msi" - msbuild .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputPath="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/}" + msbuild .\build\windows\gh.wixproj /p:SourceDir="$PWD" /p:OutputPath="$PWD" /p:OutputName="$name" /p:ProductVersion="${GITHUB_REF#refs/tags/v}" - name: Obtain signing cert id: obtain_cert env: diff --git a/build/windows/gh.wixproj b/build/windows/gh.wixproj index db5f1b0c7..5c355955d 100644 --- a/build/windows/gh.wixproj +++ b/build/windows/gh.wixproj @@ -13,6 +13,7 @@ $(DefineConstants); ProductVersion=$(ProductVersion); + ICE39 false $(MSBuildExtensionsPath)\Microsoft\WiX\v3.x\Wix.targets diff --git a/build/windows/gh.wxs b/build/windows/gh.wxs index 7d2f967d5..32eb9564b 100644 --- a/build/windows/gh.wxs +++ b/build/windows/gh.wxs @@ -5,11 +5,27 @@ - + + + + + + + + + + + + + + + + + - + @@ -21,7 +37,7 @@ - + @@ -35,6 +51,7 @@ + @@ -42,8 +59,11 @@ - - + + + + + From a6c7bd832694fd5ef7baa67ddba6cf1c5c657167 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sat, 18 Sep 2021 09:23:43 -0700 Subject: [PATCH 004/167] Resolve PR feedback * Removed license dialog * Removed RTF copy of license --- build/windows/ConvertTo-Rtf.ps1 | 24 --------------- build/windows/LICENSE.rtf | 23 -------------- build/windows/gh.wixproj | 4 +-- build/windows/gh.wxs | 5 ++- build/windows/ui.wxs | 54 +++++++++++++++++++++++++++++++++ 5 files changed, 57 insertions(+), 53 deletions(-) delete mode 100644 build/windows/ConvertTo-Rtf.ps1 delete mode 100644 build/windows/LICENSE.rtf create mode 100644 build/windows/ui.wxs diff --git a/build/windows/ConvertTo-Rtf.ps1 b/build/windows/ConvertTo-Rtf.ps1 deleted file mode 100644 index 401951cc8..000000000 --- a/build/windows/ConvertTo-Rtf.ps1 +++ /dev/null @@ -1,24 +0,0 @@ -[CmdletBinding()] -param ( - [Parameter(Mandatory=$true, Position=0)] - [string] $Path, - - [Parameter(Mandatory=$true, Position=1)] - [string] $OutFile, - - [Parameter()] - [ValidateNotNullOrEmpty()] - [string] $FontFamily = 'Arial' -) - -$rtf = "{\rtf1\ansi\deff0{\fonttbl{\f0\fcharset0 $FontFamily;}}\pard\sa200\sl200\slmult1\fs20`n" -foreach ($line in (Get-Content $Path)) { - if (!$line) { - $rtf += "\par`n" - } else { - $rtf += "$line`n" - } -} -$rtf += '}' - -$rtf | Set-Content $OutFile \ No newline at end of file diff --git a/build/windows/LICENSE.rtf b/build/windows/LICENSE.rtf deleted file mode 100644 index 185063f24..000000000 --- a/build/windows/LICENSE.rtf +++ /dev/null @@ -1,23 +0,0 @@ -{\rtf1\ansi\deff0{\fonttbl{\f0\fcharset0 Arial;}}\pard\sa200\sl200\slmult1\fs20 -MIT License -\par -Copyright (c) 2019 GitHub Inc. -\par -Permission is hereby granted, free of charge, to any person obtaining a copy -of this software and associated documentation files (the "Software"), to deal -in the Software without restriction, including without limitation the rights -to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -copies of the Software, and to permit persons to whom the Software is -furnished to do so, subject to the following conditions: -\par -The above copyright notice and this permission notice shall be included in all -copies or substantial portions of the Software. -\par -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -SOFTWARE. -} diff --git a/build/windows/gh.wixproj b/build/windows/gh.wixproj index 5c355955d..aa72d4da3 100644 --- a/build/windows/gh.wixproj +++ b/build/windows/gh.wixproj @@ -19,11 +19,9 @@ + - - - diff --git a/build/windows/gh.wxs b/build/windows/gh.wxs index 32eb9564b..1e91734f1 100644 --- a/build/windows/gh.wxs +++ b/build/windows/gh.wxs @@ -70,9 +70,8 @@ - + - - + diff --git a/build/windows/ui.wxs b/build/windows/ui.wxs new file mode 100644 index 000000000..8c534adc8 --- /dev/null +++ b/build/windows/ui.wxs @@ -0,0 +1,54 @@ + + + + + + + + + + + + + + + + + + + + + + + 1 + "1"]]> + + 1 + + NOT Installed + Installed AND PATCH + + 1 + 1 + NOT WIXUI_DONTVALIDATEPATH + "1"]]> + WIXUI_DONTVALIDATEPATH OR WIXUI_INSTALLDIR_VALID="1" + 1 + 1 + + NOT Installed + Installed AND NOT PATCH + Installed AND PATCH + + 1 + + 1 + 1 + 1 + + + + + + + \ No newline at end of file From ab668e8419f2491b41ab727d5d24abc30b112580 Mon Sep 17 00:00:00 2001 From: tdakkota Date: Mon, 1 Nov 2021 10:28:30 +0300 Subject: [PATCH 005/167] gist edit: add way to edit files from different sources This commit adds an optional parameter to set source for editing or adding files to gist. If parameter is equal to "-" then stdin will be used. --- pkg/cmd/gist/edit/edit.go | 95 +++++++++++++++----- pkg/cmd/gist/edit/edit_test.go | 156 +++++++++++++++++++++++++++++++-- 2 files changed, 222 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 174b326cd..b9db2fa54 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -5,8 +5,9 @@ import ( "encoding/json" "errors" "fmt" - "io/ioutil" + "io" "net/http" + "os" "path/filepath" "sort" "strings" @@ -32,6 +33,7 @@ type EditOptions struct { Selector string EditFilename string AddFilename string + SourceFile string } func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Command { @@ -49,11 +51,22 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } cmd := &cobra.Command{ - Use: "edit { | }", + Use: "edit { | } []", Short: "Edit one of your gists", - Args: cmdutil.ExactArgs(1, "cannot edit: gist argument required"), + Args: func(cmd *cobra.Command, args []string) error { + if len(args) < 1 { + return cmdutil.FlagErrorf("cannot edit: gist argument required") + } + if len(args) > 2 { + return cmdutil.FlagErrorf("too many arguments") + } + return nil + }, RunE: func(c *cobra.Command, args []string) error { opts.Selector = args[0] + if len(args) > 1 { + opts.SourceFile = args[1] + } if runF != nil { return runF(&opts) @@ -115,7 +128,36 @@ func editRun(opts *EditOptions) error { } if opts.AddFilename != "" { - files, err := getFilesToAdd(opts.AddFilename) + var input io.Reader + switch src := opts.SourceFile; { + case src == "-": + input = opts.IO.In + case src != "": + f, err := os.Open(src) + if err != nil { + return err + } + defer func() { + _ = f.Close() + }() + input = f + default: + f, err := os.Open(opts.AddFilename) + if err != nil { + return err + } + defer func() { + _ = f.Close() + }() + input = f + } + + content, err := io.ReadAll(input) + if err != nil { + return fmt.Errorf("read content: %w", err) + } + + files, err := getFilesToAdd(opts.AddFilename, content) if err != nil { return err } @@ -161,14 +203,32 @@ func editRun(opts *EditOptions) error { return fmt.Errorf("editing binary files not supported") } - editorCommand, err := cmdutil.DetermineEditor(opts.Config) - if err != nil { - return err - } - text, err := opts.Edit(editorCommand, filename, gistFile.Content, opts.IO) + var text string + if src := opts.SourceFile; src != "" { + if src == "-" { + data, err := io.ReadAll(opts.IO.In) + if err != nil { + return fmt.Errorf("read from stdin: %w", err) + } + text = string(data) + } else { + data, err := os.ReadFile(src) + if err != nil { + return fmt.Errorf("read %s: %w", src, err) + } + text = string(data) + } + } else { + editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } - if err != nil { - return err + data, err := opts.Edit(editorCommand, filename, gistFile.Content, opts.IO) + if err != nil { + return err + } + text = data } if text != gistFile.Content { @@ -253,20 +313,11 @@ func updateGist(apiClient *api.Client, hostname string, gist *shared.Gist) error return nil } -func getFilesToAdd(file string) (map[string]*shared.GistFile, error) { - isBinary, err := shared.IsBinaryFile(file) - if err != nil { - return nil, fmt.Errorf("failed to read file %s: %w", file, err) - } - if isBinary { +func getFilesToAdd(file string, content []byte) (map[string]*shared.GistFile, error) { + if shared.IsBinaryContents(content) { return nil, fmt.Errorf("failed to upload %s: binary file not supported", file) } - content, err := ioutil.ReadFile(file) - if err != nil { - return nil, fmt.Errorf("failed to read file %s: %w", file, err) - } - if len(content) == 0 { return nil, errors.New("file contents cannot be empty") } diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index cac99b923..efacf87ec 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -20,14 +20,11 @@ import ( ) func Test_getFilesToAdd(t *testing.T) { - fileToAdd := filepath.Join(t.TempDir(), "gist-test.txt") - err := ioutil.WriteFile(fileToAdd, []byte("hello"), 0600) + filename := "gist-test.txt" + + gf, err := getFilesToAdd(filename, []byte("hello")) require.NoError(t, err) - gf, err := getFilesToAdd(fileToAdd) - require.NoError(t, err) - - filename := filepath.Base(fileToAdd) assert.Equal(t, map[string]*shared.GistFile{ filename: { Filename: filename, @@ -65,6 +62,15 @@ func TestNewCmdEdit(t *testing.T) { AddFilename: "cool.md", }, }, + { + name: "add with source", + cli: "123 --add cool.md -", + wants: EditOptions{ + Selector: "123", + AddFilename: "cool.md", + SourceFile: "-", + }, + }, } for _, tt := range tests { @@ -106,6 +112,7 @@ func Test_editRun(t *testing.T) { httpStubs func(*httpmock.Registry) askStubs func(*prompt.AskStubber) nontty bool + stdin string wantErr string wantParams map[string]interface{} }{ @@ -262,6 +269,140 @@ func Test_editRun(t *testing.T) { AddFilename: fileToAdd, }, }, + { + name: "add file to existing gist from source parameter", + gist: &shared.Gist{ + ID: "1234", + Files: map[string]*shared.GistFile{ + "sample.txt": { + Filename: "sample.txt", + Content: "bwhiizzzbwhuiiizzzz", + Type: "text/plain", + }, + }, + Owner: &shared.GistOwner{Login: "octocat"}, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("POST", "gists/1234"), + httpmock.StatusStringResponse(201, "{}")) + }, + opts: &EditOptions{ + AddFilename: "from_source.txt", + SourceFile: fileToAdd, + }, + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "from_source.txt": map[string]interface{}{ + "content": "hello", + "filename": "from_source.txt", + }, + }, + }, + }, + { + name: "add file to existing gist from stdin", + gist: &shared.Gist{ + ID: "1234", + Files: map[string]*shared.GistFile{ + "sample.txt": { + Filename: "sample.txt", + Content: "bwhiizzzbwhuiiizzzz", + Type: "text/plain", + }, + }, + Owner: &shared.GistOwner{Login: "octocat"}, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("POST", "gists/1234"), + httpmock.StatusStringResponse(201, "{}")) + }, + opts: &EditOptions{ + AddFilename: "from_source.txt", + SourceFile: "-", + }, + stdin: "data from stdin", + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "from_source.txt": map[string]interface{}{ + "content": "data from stdin", + "filename": "from_source.txt", + }, + }, + }, + }, + { + name: "edit gist using file from source parameter", + gist: &shared.Gist{ + ID: "1234", + Files: map[string]*shared.GistFile{ + "sample.txt": { + Filename: "sample.txt", + Content: "bwhiizzzbwhuiiizzzz", + Type: "text/plain", + }, + }, + Owner: &shared.GistOwner{Login: "octocat"}, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("POST", "gists/1234"), + httpmock.StatusStringResponse(201, "{}")) + }, + opts: &EditOptions{ + SourceFile: fileToAdd, + }, + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "sample.txt": map[string]interface{}{ + "content": "hello", + "filename": "sample.txt", + "type": "text/plain", + }, + }, + }, + }, + { + name: "edit gist using stdin", + gist: &shared.Gist{ + ID: "1234", + Files: map[string]*shared.GistFile{ + "sample.txt": { + Filename: "sample.txt", + Content: "bwhiizzzbwhuiiizzzz", + Type: "text/plain", + }, + }, + Owner: &shared.GistOwner{Login: "octocat"}, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("POST", "gists/1234"), + httpmock.StatusStringResponse(201, "{}")) + }, + opts: &EditOptions{ + SourceFile: "-", + }, + stdin: "data from stdin", + wantParams: map[string]interface{}{ + "description": "", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "sample.txt": map[string]interface{}{ + "content": "data from stdin", + "filename": "sample.txt", + "type": "text/plain", + }, + }, + }, + }, } for _, tt := range tests { @@ -297,7 +438,8 @@ func Test_editRun(t *testing.T) { tt.opts.HttpClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - io, _, stdout, stderr := iostreams.Test() + io, stdin, stdout, stderr := iostreams.Test() + stdin.WriteString(tt.stdin) io.SetStdoutTTY(!tt.nontty) io.SetStdinTTY(!tt.nontty) tt.opts.IO = io From f30e76aab8a1584bb3e78bfad691897990f76bb0 Mon Sep 17 00:00:00 2001 From: Ben Steadman Date: Tue, 16 Nov 2021 20:05:54 +0000 Subject: [PATCH 006/167] Support editing gist description Add a --desc flag to gh gist edit to support editing gist descriptions. This flag can be used in combination with the other gist editing flags to edit the description whilst also adding/editing files. Signed-off-by: Ben Steadman --- pkg/cmd/gist/edit/edit.go | 22 +++++++++++------- pkg/cmd/gist/edit/edit_test.go | 41 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 174b326cd..a43cf9a5b 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -32,6 +32,7 @@ type EditOptions struct { Selector string EditFilename string AddFilename string + Description string } func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Command { @@ -64,6 +65,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } cmd.Flags().StringVarP(&opts.AddFilename, "add", "a", "", "Add a new file to the gist") + cmd.Flags().StringVarP(&opts.Description, "desc", "d", "", "New description for the gist") cmd.Flags().StringVarP(&opts.EditFilename, "filename", "f", "", "Select a file to edit") return cmd @@ -114,6 +116,12 @@ func editRun(opts *EditOptions) error { return fmt.Errorf("You do not own this gist.") } + shouldUpdate := false + if opts.Description != "" { + shouldUpdate = true + gist.Description = opts.Description + } + if opts.AddFilename != "" { files, err := getFilesToAdd(opts.AddFilename) if err != nil { @@ -166,7 +174,6 @@ func editRun(opts *EditOptions) error { return err } text, err := opts.Edit(editorCommand, filename, gistFile.Content, opts.IO) - if err != nil { return err } @@ -215,16 +222,15 @@ func editRun(opts *EditOptions) error { } } - if len(filesToUpdate) == 0 { + if len(filesToUpdate) > 0 { + shouldUpdate = true + } + + if !shouldUpdate { return nil } - err = updateGist(apiClient, host, gist) - if err != nil { - return err - } - - return nil + return updateGist(apiClient, host, gist) } func updateGist(apiClient *api.Client, hostname string, gist *shared.Gist) error { diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index cac99b923..62b6fcb25 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -65,6 +65,14 @@ func TestNewCmdEdit(t *testing.T) { AddFilename: "cool.md", }, }, + { + name: "description", + cli: `123 --desc "my new description"`, + wants: EditOptions{ + Selector: "123", + Description: "my new description", + }, + }, } for _, tt := range tests { @@ -262,6 +270,39 @@ func Test_editRun(t *testing.T) { AddFilename: fileToAdd, }, }, + { + name: "change description", + gist: &shared.Gist{ + ID: "1234", + Description: "my old description", + Files: map[string]*shared.GistFile{ + "sample.txt": { + Filename: "sample.txt", + Type: "text/plain", + }, + }, + Owner: &shared.GistOwner{Login: "octocat"}, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("POST", "gists/1234"), + httpmock.StatusStringResponse(201, "{}")) + }, + wantParams: map[string]interface{}{ + "description": "my new description", + "updated_at": "0001-01-01T00:00:00Z", + "public": false, + "files": map[string]interface{}{ + "sample.txt": map[string]interface{}{ + "content": "new file content", + "filename": "sample.txt", + "type": "text/plain", + }, + }, + }, + opts: &EditOptions{ + Description: "my new description", + }, + }, } for _, tt := range tests { From 54d92facbf64f6acbca33f3b928688d638d9a9d9 Mon Sep 17 00:00:00 2001 From: Gowtham Munukutla Date: Fri, 10 Dec 2021 22:59:35 +0530 Subject: [PATCH 007/167] add flag to set fork-name during repo fork. Tests WIP --- api/queries_repo.go | 32 ++++++++++++++++++ git/remote.go | 1 + pkg/cmd/repo/fork/fork.go | 10 ++++++ pkg/cmd/repo/rename/http.go | 61 ----------------------------------- pkg/cmd/repo/rename/rename.go | 7 +++- 5 files changed, 49 insertions(+), 62 deletions(-) delete mode 100644 pkg/cmd/repo/rename/http.go diff --git a/api/queries_repo.go b/api/queries_repo.go index c6159f205..22079fb39 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/cli/cli/v2/internal/ghinstance" "io" "net/http" "sort" @@ -524,6 +525,37 @@ func ForkRepo(client *Client, repo ghrepo.Interface, org string) (*Repository, e }, nil } +// RenameRepo renames the repository on GitHub and returns the renamed repository +func RenameRepo(client *Client, repo ghrepo.Interface, newRepoName string) (*Repository, error) { + input := map[string]string{"name": newRepoName} + body := &bytes.Buffer{} + enc := json.NewEncoder(body) + if err := enc.Encode(input); err != nil { + return nil, err + } + + path := fmt.Sprintf("%srepos/%s", + ghinstance.RESTPrefix(repo.RepoHost()), + ghrepo.FullName(repo)) + + result := repositoryV3{} + err := client.REST(repo.RepoHost(), "PATCH", path, body, &result) + if err != nil { + return nil, err + } + + return &Repository{ + ID: result.NodeID, + Name: result.Name, + CreatedAt: result.CreatedAt, + Owner: RepositoryOwner{ + Login: result.Owner.Login, + }, + ViewerPermission: "WRITE", + hostname: repo.RepoHost(), + }, nil +} + // RepoFindForks finds forks of the repo that are affiliated with the viewer func RepoFindForks(client *Client, repo ghrepo.Interface, limit int) ([]*Repository, error) { result := struct { diff --git a/git/remote.go b/git/remote.go index bea81da90..7a5890cb6 100644 --- a/git/remote.go +++ b/git/remote.go @@ -153,6 +153,7 @@ func AddRemote(name, u string) (*Remote, error) { } func UpdateRemoteURL(name, u string) error { + fmt.Println(name, u, "=====================name u==============") addCmd, err := GitCommand("remote", "set-url", name, u) if err != nil { return err diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 773f46641..1295482ca 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -39,6 +39,7 @@ type ForkOptions struct { PromptRemote bool RemoteName string Organization string + ForkName string Rename bool } @@ -115,6 +116,7 @@ Additional 'git clone' flags can be passed in by listing them after '--'.`, cmd.Flags().BoolVar(&opts.Remote, "remote", false, "Add remote for fork {true|false}") cmd.Flags().StringVar(&opts.RemoteName, "remote-name", defaultRemoteName, "Specify a name for a fork's new remote.") cmd.Flags().StringVar(&opts.Organization, "org", "", "Create the fork in an organization") + cmd.Flags().StringVar(&opts.ForkName, "fork-name", "", "Specify a name for the forked repo") return cmd } @@ -180,6 +182,14 @@ func forkRun(opts *ForkOptions) error { return fmt.Errorf("failed to fork: %w", err) } + // Rename the forked repo if ForkName is specified in opts. + if opts.ForkName != "" { + forkedRepo, err = api.RenameRepo(apiClient, forkedRepo, opts.ForkName) + if err != nil { + return err + } + } + // This is weird. There is not an efficient way to determine via the GitHub API whether or not a // given user has forked a given repo. We noticed, also, that the create fork API endpoint just // returns the fork repo data even if it already exists -- with no change in status code or diff --git a/pkg/cmd/repo/rename/http.go b/pkg/cmd/repo/rename/http.go deleted file mode 100644 index 1e5d73477..000000000 --- a/pkg/cmd/repo/rename/http.go +++ /dev/null @@ -1,61 +0,0 @@ -package rename - -import ( - "bytes" - "encoding/json" - "fmt" - "io/ioutil" - "net/http" - - "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/ghrepo" -) - -func apiRename(client *http.Client, repo ghrepo.Interface, newRepoName string) (ghrepo.Interface, error) { - input := map[string]string{"name": newRepoName} - body, err := json.Marshal(input) - if err != nil { - return nil, err - } - - path := fmt.Sprintf("%srepos/%s", - ghinstance.RESTPrefix(repo.RepoHost()), - ghrepo.FullName(repo)) - - request, err := http.NewRequest("PATCH", path, bytes.NewBuffer(body)) - if err != nil { - return nil, err - } - - request.Header.Set("Content-Type", "application/json; charset=utf-8") - - resp, err := client.Do(request) - if err != nil { - return nil, err - } - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - defer resp.Body.Close() - b, err := ioutil.ReadAll(resp.Body) - if err != nil { - return nil, err - } - - result := struct { - Name string - Owner struct { - Login string - } - }{} - if err := json.Unmarshal(b, &result); err != nil { - return nil, fmt.Errorf("error unmarshaling response: %w", err) - } - - newRepo := ghrepo.NewWithHost(result.Owner.Login, result.Name, repo.RepoHost()) - - return newRepo, nil -} diff --git a/pkg/cmd/repo/rename/rename.go b/pkg/cmd/repo/rename/rename.go index d8b0e8123..a0f6ee4bf 100644 --- a/pkg/cmd/repo/rename/rename.go +++ b/pkg/cmd/repo/rename/rename.go @@ -2,6 +2,7 @@ package rename import ( "fmt" + "github.com/cli/cli/v2/api" "net/http" "github.com/AlecAivazis/survey/v2" @@ -114,7 +115,9 @@ func renameRun(opts *RenameOptions) error { } } - newRepo, err := apiRename(httpClient, currRepo, newRepoName) + apiClient := api.NewClientFromHTTP(httpClient) + + newRepo, err := api.RenameRepo(apiClient, currRepo, newRepoName) if err != nil { return err } @@ -124,6 +127,8 @@ func renameRun(opts *RenameOptions) error { fmt.Fprintf(opts.IO.Out, "%s Renamed repository %s\n", cs.SuccessIcon(), ghrepo.FullName(newRepo)) } + fmt.Println(opts.HasRepoOverride, "=========erpo override=============") + if opts.HasRepoOverride { return nil } From 3fb4c481dc55885a57bd64c870822cbba1744b73 Mon Sep 17 00:00:00 2001 From: Gowtham Munukutla Date: Sat, 11 Dec 2021 10:17:04 +0530 Subject: [PATCH 008/167] modify tests --- api/client.go | 1 + git/remote.go | 1 - pkg/cmd/repo/fork/fork_test.go | 82 ++++++++++++++++++++++++++++++ pkg/cmd/repo/rename/rename.go | 6 +-- pkg/cmd/repo/rename/rename_test.go | 10 ++-- 5 files changed, 91 insertions(+), 9 deletions(-) diff --git a/api/client.go b/api/client.go index 3fcf0d930..a3bf486ad 100644 --- a/api/client.go +++ b/api/client.go @@ -310,6 +310,7 @@ func (c Client) REST(hostname string, method string, p string, body io.Reader, d if err != nil { return err } + err = json.Unmarshal(b, &data) if err != nil { return err diff --git a/git/remote.go b/git/remote.go index 7a5890cb6..bea81da90 100644 --- a/git/remote.go +++ b/git/remote.go @@ -153,7 +153,6 @@ func AddRemote(name, u string) (*Remote, error) { } func UpdateRemoteURL(name, u string) error { - fmt.Println(name, u, "=====================name u==============") addCmd, err := GitCommand("remote", "set-url", name, u) if err != nil { return err diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 775152440..4028ff929 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -132,6 +132,16 @@ func TestNewCmdFork(t *testing.T) { wantErr: true, errMsg: "unknown flag: --depth\nSeparate git clone flags with '--'.", }, + { + name: "with fork name", + cli: "--fork-name new-fork", + wants: ForkOptions{ + Remote: false, + RemoteName: "origin", + ForkName: "new-fork", + Rename: false, + }, + }, } for _, tt := range tests { @@ -529,6 +539,78 @@ func TestRepoFork(t *testing.T) { cs.Register(`git -C REPO remote add -f upstream https://github\.com/OWNER/REPO\.git`, 0, "") }, }, + { + name: "non tty repo arg with fork-name", + opts: &ForkOptions{ + Repository: "someone/REPO", + Clone: false, + ForkName: "NEW_REPO", + }, + tty: false, + httpStubs: func(reg *httpmock.Registry) { + forkResult := `{ + "node_id": "123", + "name": "REPO", + "clone_url": "https://github.com/OWNER/REPO.git", + "created_at": "2011-01-26T19:01:12Z", + "owner": { + "login": "OWNER" + } + }` + renameResult := `{ + "node_id": "1234", + "name": "NEW_REPO", + "clone_url": "https://github.com/OWNER/NEW_REPO.git", + "created_at": "2012-01-26T19:01:12Z", + "owner": { + "login": "OWNER" + } + }` + reg.Register( + httpmock.REST("POST", "repos/someone/REPO/forks"), + httpmock.StringResponse(forkResult)) + reg.Register( + httpmock.REST("PATCH", "repos/OWNER/REPO"), + httpmock.StringResponse(renameResult)) + }, + wantErrOut: "", + }, + { + name: "tty repo arg with fork-name", + opts: &ForkOptions{ + Repository: "someone/REPO", + Clone: false, + ForkName: "NEW_REPO", + }, + tty: true, + httpStubs: func(reg *httpmock.Registry) { + forkResult := `{ + "node_id": "123", + "name": "REPO", + "clone_url": "https://github.com/OWNER/REPO.git", + "created_at": "2011-01-26T19:01:12Z", + "owner": { + "login": "OWNER" + } + }` + renameResult := `{ + "node_id": "1234", + "name": "NEW_REPO", + "clone_url": "https://github.com/OWNER/NEW_REPO.git", + "created_at": "2012-01-26T19:01:12Z", + "owner": { + "login": "OWNER" + } + }` + reg.Register( + httpmock.REST("POST", "repos/someone/REPO/forks"), + httpmock.StringResponse(forkResult)) + reg.Register( + httpmock.REST("PATCH", "repos/OWNER/REPO"), + httpmock.StringResponse(renameResult)) + }, + wantErrOut: "✓ Created fork OWNER/NEW_REPO\n", + }, } for _, tt := range tests { diff --git a/pkg/cmd/repo/rename/rename.go b/pkg/cmd/repo/rename/rename.go index a0f6ee4bf..54747cff9 100644 --- a/pkg/cmd/repo/rename/rename.go +++ b/pkg/cmd/repo/rename/rename.go @@ -122,18 +122,18 @@ func renameRun(opts *RenameOptions) error { return err } + renamedRepo := ghrepo.New(newRepo.Owner.Login, newRepo.Name) + cs := opts.IO.ColorScheme() if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.Out, "%s Renamed repository %s\n", cs.SuccessIcon(), ghrepo.FullName(newRepo)) } - fmt.Println(opts.HasRepoOverride, "=========erpo override=============") - if opts.HasRepoOverride { return nil } - remote, err := updateRemote(currRepo, newRepo, opts) + remote, err := updateRemote(currRepo, renamedRepo, opts) if err != nil { fmt.Fprintf(opts.IO.ErrOut, "%s Warning: unable to update remote %q: %v\n", cs.WarningIcon(), remote.Name, err) } else if opts.IO.IsStdoutTTY() { diff --git a/pkg/cmd/repo/rename/rename_test.go b/pkg/cmd/repo/rename/rename_test.go index 042579613..549abc731 100644 --- a/pkg/cmd/repo/rename/rename_test.go +++ b/pkg/cmd/repo/rename/rename_test.go @@ -122,7 +122,7 @@ func TestRenameRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(204, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) + httpmock.StatusStringResponse(200, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) }, execStubs: func(cs *run.CommandStubber) { cs.Register(`git remote set-url origin https://github.com/OWNER/NEW_REPO.git`, 0, "") @@ -141,7 +141,7 @@ func TestRenameRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(204, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) + httpmock.StatusStringResponse(200, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) }, tty: true, }, @@ -154,7 +154,7 @@ func TestRenameRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(204, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) + httpmock.StatusStringResponse(200, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) }, execStubs: func(cs *run.CommandStubber) { cs.Register(`git remote set-url origin https://github.com/OWNER/NEW_REPO.git`, 0, "") @@ -169,7 +169,7 @@ func TestRenameRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(204, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) + httpmock.StatusStringResponse(200, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) }, execStubs: func(cs *run.CommandStubber) { cs.Register(`git remote set-url origin https://github.com/OWNER/NEW_REPO.git`, 0, "") @@ -189,7 +189,7 @@ func TestRenameRun(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(204, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) + httpmock.StatusStringResponse(200, `{"name":"NEW_REPO","owner":{"login":"OWNER"}}`)) }, execStubs: func(cs *run.CommandStubber) { cs.Register(`git remote set-url origin https://github.com/OWNER/NEW_REPO.git`, 0, "") From 9416056da18bb1cc731917a32d6a3c3e4fe613d5 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 13 Dec 2021 15:18:32 -0700 Subject: [PATCH 009/167] allow combining os.Stdin and os.Stdout as an io.ReadWriteCloser --- pkg/cmd/codespace/ssh.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 840386d5c..0a1425a87 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -277,3 +277,32 @@ func (fl *fileLogger) Name() string { func (fl *fileLogger) Close() error { return fl.f.Close() } + +type combinedReadWriteCloser struct { + reader *os.File + writer *os.File +} + +func newCombinedReadWriteCloser(reader *os.File, writer *os.File) (crwc *combinedReadWriteCloser) { + return &combinedReadWriteCloser{ + reader: reader, + writer: writer, + } +} + +func (crwc *combinedReadWriteCloser) Read(p []byte) (n int, err error) { + return crwc.reader.Read(p) +} + +func (crwc *combinedReadWriteCloser) Write(p []byte) (n int, err error) { + return crwc.writer.Write(p) +} + +func (crwc *combinedReadWriteCloser) Close() error { + werr := crwc.writer.Close() + rerr := crwc.reader.Close() + if werr != nil { + return werr + } + return rerr +} From 81b658ea750ff05733f3bc6bf51bdcfe1e81be6c Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 13 Dec 2021 15:19:25 -0700 Subject: [PATCH 010/167] use go-multierror to combine read/write close errors --- go.mod | 1 + go.sum | 2 ++ pkg/cmd/codespace/ssh.go | 13 ++++++++----- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index fe082e3a3..e79aff389 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/google/go-cmp v0.5.6 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/gorilla/websocket v1.4.2 + github.com/hashicorp/go-multierror v1.0.0 github.com/hashicorp/go-version v1.3.0 github.com/henvic/httpretty v0.0.6 github.com/itchyny/gojq v0.12.6 diff --git a/go.sum b/go.sum index fd9b7ae9d..bd9272641 100644 --- a/go.sum +++ b/go.sum @@ -192,10 +192,12 @@ github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/ad github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= +github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= +github.com/hashicorp/go-multierror v1.0.0 h1:iVjPR7a6H0tWELX5NxNe7bYopibicUzc7uPribsnS6o= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa6eBIzfwKfwNnHU= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 0a1425a87..738b352ba 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -16,6 +16,7 @@ import ( "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/liveshare" + "github.com/hashicorp/go-multierror" "github.com/spf13/cobra" ) @@ -299,10 +300,12 @@ func (crwc *combinedReadWriteCloser) Write(p []byte) (n int, err error) { } func (crwc *combinedReadWriteCloser) Close() error { - werr := crwc.writer.Close() - rerr := crwc.reader.Close() - if werr != nil { - return werr + var errs error + if err := crwc.writer.Close(); err != nil { + errs = multierror.Append(errs, err) } - return rerr + if err := crwc.reader.Close(); err != nil { + errs = multierror.Append(errs, err) + } + return errs } From c5e553e4001e5ea90d6f3a7db33dea3b754d43c3 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 13 Dec 2021 15:27:44 -0700 Subject: [PATCH 011/167] implement `gh cs ssh --stdio` --- pkg/cmd/codespace/ssh.go | 91 +++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 738b352ba..a397961fc 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -26,6 +26,7 @@ type sshOptions struct { serverPort int debug bool debugFile string + stdio bool scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } @@ -47,6 +48,11 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") + // TODO: alternate name: "--proxy"? something else? + // also need to make this mutually exclusive with profile/server-port + // should probably require --codespace as well + sshCmd.Flags().BoolVarP(&opts.stdio, "stdio", "", false, "Proxy sshd connection to stdio") + return sshCmd } @@ -102,49 +108,56 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e return fmt.Errorf("error getting ssh server details: %w", err) } - localSSHServerPort := opts.serverPort - usingCustomPort := localSSHServerPort != 0 // suppress log of command line in Shell - - // Ensure local port is listening before client (Shell) connects. - // Unless the user specifies a server port, localSSHServerPort is 0 - // and thus the client will pick a random port. - listen, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", localSSHServerPort)) - if err != nil { - return err - } - defer listen.Close() - localSSHServerPort = listen.Addr().(*net.TCPAddr).Port - - connectDestination := opts.profile - if connectDestination == "" { - connectDestination = fmt.Sprintf("%s@localhost", sshUser) - } - - tunnelClosed := make(chan error, 1) - go func() { + if opts.stdio { fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) - tunnelClosed <- fwd.ForwardToListener(ctx, listen) // always non-nil - }() - - shellClosed := make(chan error, 1) - go func() { - var err error - if opts.scpArgs != nil { - err = codespaces.Copy(ctx, opts.scpArgs, localSSHServerPort, connectDestination) - } else { - err = codespaces.Shell(ctx, a.errLogger, sshArgs, localSSHServerPort, connectDestination, usingCustomPort) - } - shellClosed <- err - }() - - select { - case err := <-tunnelClosed: + stdio := newCombinedReadWriteCloser(os.Stdin, os.Stdout) + err := fwd.Forward(ctx, stdio) // always non-nil return fmt.Errorf("tunnel closed: %w", err) - case err := <-shellClosed: + } else { + localSSHServerPort := opts.serverPort + usingCustomPort := localSSHServerPort != 0 // suppress log of command line in Shell + + // Ensure local port is listening before client (Shell) connects. + // Unless the user specifies a server port, localSSHServerPort is 0 + // and thus the client will pick a random port. + listen, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", localSSHServerPort)) if err != nil { - return fmt.Errorf("shell closed: %w", err) + return err + } + defer listen.Close() + localSSHServerPort = listen.Addr().(*net.TCPAddr).Port + + connectDestination := opts.profile + if connectDestination == "" { + connectDestination = fmt.Sprintf("%s@localhost", sshUser) + } + + tunnelClosed := make(chan error, 1) + go func() { + fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) + tunnelClosed <- fwd.ForwardToListener(ctx, listen) // always non-nil + }() + + shellClosed := make(chan error, 1) + go func() { + var err error + if opts.scpArgs != nil { + err = codespaces.Copy(ctx, opts.scpArgs, localSSHServerPort, connectDestination) + } else { + err = codespaces.Shell(ctx, a.errLogger, sshArgs, localSSHServerPort, connectDestination, usingCustomPort) + } + shellClosed <- err + }() + + select { + case err := <-tunnelClosed: + return fmt.Errorf("tunnel closed: %w", err) + case err := <-shellClosed: + if err != nil { + return fmt.Errorf("shell closed: %w", err) + } + return nil // success } - return nil // success } } From 4306762f8bb4ac9038b888054859e7ec9b37f0f3 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 13 Dec 2021 18:43:56 -0700 Subject: [PATCH 012/167] factor out openSshSession() helper function --- pkg/cmd/codespace/ssh.go | 57 ++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index a397961fc..133483ebb 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -62,23 +62,6 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e ctx, cancel := context.WithCancel(ctx) defer cancel() - codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) - if err != nil { - return fmt.Errorf("get or choose codespace: %w", err) - } - - // TODO(josebalius): We can fetch the user in parallel to everything else - // we should convert this call and others to happen async - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return fmt.Errorf("error getting user: %w", err) - } - - authkeys := make(chan error, 1) - go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) - }() - liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) @@ -91,16 +74,12 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e a.errLogger.Printf("Debug file located at: %s", debugLogger.Name()) } - session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) + session, err := openSshSession(ctx, a, opts.codespace, liveshareLogger) if err != nil { return fmt.Errorf("error connecting to codespace: %w", err) } defer safeClose(session, &err) - if err := <-authkeys; err != nil { - return err - } - a.StartProgressIndicatorWithLabel("Fetching SSH Details") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) a.StopProgressIndicator() @@ -161,6 +140,40 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } +func openSshSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { + codespace, err := getOrChooseCodespace(ctx, a.apiClient, csName) + if err != nil { + return nil, fmt.Errorf("get or choose codespace: %w", err) + } + + // TODO(josebalius): We can fetch the user in parallel to everything else + // we should convert this call and others to happen async + user, err := a.apiClient.GetUser(ctx) + if err != nil { + return nil, fmt.Errorf("error getting user: %w", err) + } + + authkeys := make(chan error, 1) + go func() { + authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) + }() + + if liveshareLogger == nil { + liveshareLogger = noopLogger() + } + + session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) + if err != nil { + return nil, fmt.Errorf("error connecting to codespace: %w", err) + } + + if err := <-authkeys; err != nil { + return nil, err + } + + return session, nil +} + type cpOptions struct { sshOptions recursive bool // -r From 77650006012724a3a0d6c770402af813d798b69c Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Tue, 14 Dec 2021 16:26:25 -0700 Subject: [PATCH 013/167] add `gh cs ssh` openssh config file generator --- pkg/cmd/codespace/ssh.go | 166 +++++++++++++++++++++++++++++---------- 1 file changed, 125 insertions(+), 41 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 133483ebb..7ae079272 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -11,6 +11,7 @@ import ( "os" "path/filepath" "strings" + "text/template" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" @@ -27,6 +28,7 @@ type sshOptions struct { debug bool debugFile string stdio bool + config bool scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } @@ -52,6 +54,7 @@ func newSSHCmd(app *App) *cobra.Command { // also need to make this mutually exclusive with profile/server-port // should probably require --codespace as well sshCmd.Flags().BoolVarP(&opts.stdio, "stdio", "", false, "Proxy sshd connection to stdio") + sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") return sshCmd } @@ -62,6 +65,10 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e ctx, cancel := context.WithCancel(ctx) defer cancel() + if opts.config { + return a.ListOpensshConfig(ctx) + } + liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) @@ -92,52 +99,129 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e stdio := newCombinedReadWriteCloser(os.Stdin, os.Stdout) err := fwd.Forward(ctx, stdio) // always non-nil return fmt.Errorf("tunnel closed: %w", err) - } else { - localSSHServerPort := opts.serverPort - usingCustomPort := localSSHServerPort != 0 // suppress log of command line in Shell + } - // Ensure local port is listening before client (Shell) connects. - // Unless the user specifies a server port, localSSHServerPort is 0 - // and thus the client will pick a random port. - listen, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", localSSHServerPort)) + localSSHServerPort := opts.serverPort + usingCustomPort := localSSHServerPort != 0 // suppress log of command line in Shell + + // Ensure local port is listening before client (Shell) connects. + // Unless the user specifies a server port, localSSHServerPort is 0 + // and thus the client will pick a random port. + listen, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", localSSHServerPort)) + if err != nil { + return err + } + defer listen.Close() + localSSHServerPort = listen.Addr().(*net.TCPAddr).Port + + connectDestination := opts.profile + if connectDestination == "" { + connectDestination = fmt.Sprintf("%s@localhost", sshUser) + } + + tunnelClosed := make(chan error, 1) + go func() { + fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) + tunnelClosed <- fwd.ForwardToListener(ctx, listen) // always non-nil + }() + + shellClosed := make(chan error, 1) + go func() { + var err error + if opts.scpArgs != nil { + err = codespaces.Copy(ctx, opts.scpArgs, localSSHServerPort, connectDestination) + } else { + err = codespaces.Shell(ctx, a.errLogger, sshArgs, localSSHServerPort, connectDestination, usingCustomPort) + } + shellClosed <- err + }() + + select { + case err := <-tunnelClosed: + return fmt.Errorf("tunnel closed: %w", err) + case err := <-shellClosed: if err != nil { + return fmt.Errorf("shell closed: %w", err) + } + return nil // success + } +} + +func (a *App) ListOpensshConfig(ctx context.Context) error { + a.StartProgressIndicatorWithLabel("Fetching codespaces") + codespaces, err := a.apiClient.ListCodespaces(ctx, -1) + a.StopProgressIndicator() + if err != nil { + return fmt.Errorf("error getting codespaces: %w", err) + } + + t, err := template.New("ssh_config").Parse(`Host cs.{{.Name}}.{{.EscapedRef}} + User {{.SshUser}} + ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio + UserKnownHostsFile=/dev/null + StrictHostKeyChecking no + LogLevel quiet + ControlMaster auto + +`) + if err != nil { + return fmt.Errorf("error formatting template: %w", err) + } + + ghexec, err := os.Executable() + if err != nil { + return err + } + + sshUsers := map[string]string{} + + for _, cs := range codespaces { + + if cs.State != "Available" { + fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) + continue + } + + var sshUser string + var ok bool + + if sshUser, ok = sshUsers[cs.Repository.FullName]; !ok { + session, err := openSshSession(ctx, a, cs.Name, nil) + if err != nil { + fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) + continue + } + defer session.Close() + + a.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching SSH Details for %s", cs.Name)) + _, sshUser, err = session.StartSSHServer(ctx) + a.StopProgressIndicator() + if err != nil { + fmt.Fprintf(os.Stderr, "error getting ssh server details: %v", err) + continue + } + sshUsers[cs.Repository.FullName] = sshUser + } + + conf := codespaceSshConfig{ + Name: cs.Name, + EscapedRef: strings.ReplaceAll(cs.GitStatus.Ref, "/", "-"), + SshUser: sshUser, + GHExec: ghexec, + } + if err := t.Execute(a.io.Out, conf); err != nil { return err } - defer listen.Close() - localSSHServerPort = listen.Addr().(*net.TCPAddr).Port - - connectDestination := opts.profile - if connectDestination == "" { - connectDestination = fmt.Sprintf("%s@localhost", sshUser) - } - - tunnelClosed := make(chan error, 1) - go func() { - fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) - tunnelClosed <- fwd.ForwardToListener(ctx, listen) // always non-nil - }() - - shellClosed := make(chan error, 1) - go func() { - var err error - if opts.scpArgs != nil { - err = codespaces.Copy(ctx, opts.scpArgs, localSSHServerPort, connectDestination) - } else { - err = codespaces.Shell(ctx, a.errLogger, sshArgs, localSSHServerPort, connectDestination, usingCustomPort) - } - shellClosed <- err - }() - - select { - case err := <-tunnelClosed: - return fmt.Errorf("tunnel closed: %w", err) - case err := <-shellClosed: - if err != nil { - return fmt.Errorf("shell closed: %w", err) - } - return nil // success - } } + + return nil +} + +type codespaceSshConfig struct { + Name string + EscapedRef string + SshUser string + GHExec string } func openSshSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { From 92403f3a2fbef66d9ba5a835b73c6a7e8952c169 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Tue, 14 Dec 2021 17:54:09 -0700 Subject: [PATCH 014/167] check for incompatible command line options --- pkg/cmd/codespace/ssh.go | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 7ae079272..68be66187 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -4,6 +4,7 @@ package codespace import ( "context" + "errors" "fmt" "io/ioutil" "log" @@ -38,6 +39,38 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd := &cobra.Command{ Use: "ssh [...] [-- ...] []", Short: "SSH into a codespace", + PreRunE: func(c *cobra.Command, args []string) error { + f := c.Flags() + codespaceFlag := f.Lookup("codespace") + configFlag := f.Lookup("config") + portFlag := f.Lookup("server-port") + profileFlag := f.Lookup("profile") + stdioFlag := f.Lookup("stdio") + + if stdioFlag.Changed { + if !codespaceFlag.Changed { + return errors.New("`--stdio` requires explicit `--codespace`") + } + if configFlag.Changed { + return errors.New("cannot use `--stdio` with `--config`") + } + if portFlag.Changed { + return errors.New("cannot use `--stdio` with `--server-port`") + } + if profileFlag.Changed { + return errors.New("cannot use `--stdio` with `--profile`") + } + } + if configFlag.Changed { + if profileFlag.Changed { + return errors.New("cannot use `--config` with `--profile`") + } + if portFlag.Changed { + return errors.New("cannot use `--config` with `--server-port`") + } + } + return nil + }, RunE: func(cmd *cobra.Command, args []string) error { return app.SSH(cmd.Context(), args, opts) }, @@ -49,10 +82,6 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") - - // TODO: alternate name: "--proxy"? something else? - // also need to make this mutually exclusive with profile/server-port - // should probably require --codespace as well sshCmd.Flags().BoolVarP(&opts.stdio, "stdio", "", false, "Proxy sshd connection to stdio") sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") From 0e6abda73b1186f02c769bc1a65ce47f8b6b2d35 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Tue, 14 Dec 2021 17:54:25 -0700 Subject: [PATCH 015/167] allow generating ssh config for a single codespace --- pkg/cmd/codespace/ssh.go | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 68be66187..5c551eee0 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -16,6 +16,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" + "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/liveshare" "github.com/hashicorp/go-multierror" @@ -72,7 +73,11 @@ func newSSHCmd(app *App) *cobra.Command { return nil }, RunE: func(cmd *cobra.Command, args []string) error { - return app.SSH(cmd.Context(), args, opts) + if opts.config { + return app.ListOpensshConfig(cmd.Context(), opts) + } else { + return app.SSH(cmd.Context(), args, opts) + } }, DisableFlagsInUseLine: true, } @@ -94,10 +99,6 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e ctx, cancel := context.WithCancel(ctx) defer cancel() - if opts.config { - return a.ListOpensshConfig(ctx) - } - liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) @@ -176,12 +177,24 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) ListOpensshConfig(ctx context.Context) error { - a.StartProgressIndicatorWithLabel("Fetching codespaces") - codespaces, err := a.apiClient.ListCodespaces(ctx, -1) - a.StopProgressIndicator() +func (a *App) ListOpensshConfig(ctx context.Context, opts sshOptions) error { + // Ensure all child tasks (e.g. port forwarding) terminate before return. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + var err error + var codespaces []*api.Codespace + if opts.codespace == "" { + a.StartProgressIndicatorWithLabel("Fetching codespaces") + codespaces, err = a.apiClient.ListCodespaces(ctx, -1) + a.StopProgressIndicator() + } else { + var codespace *api.Codespace + codespace, err = getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + codespaces = []*api.Codespace{codespace} + } if err != nil { - return fmt.Errorf("error getting codespaces: %w", err) + return fmt.Errorf("error getting codespace info: %w", err) } t, err := template.New("ssh_config").Parse(`Host cs.{{.Name}}.{{.EscapedRef}} From c9d0085e57a153d30722f4f99d431d7078db9da7 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 15:09:52 -0700 Subject: [PATCH 016/167] move `gh cs ssh --config` into a separate `gh cs ssh config` command We could also move this to a toplevel command, but I don't want to pollute that namespace too much. Open to suggestions. --- pkg/cmd/codespace/ssh.go | 55 +++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 5c551eee0..b6c2d6596 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -30,7 +30,6 @@ type sshOptions struct { debug bool debugFile string stdio bool - config bool scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } @@ -43,7 +42,6 @@ func newSSHCmd(app *App) *cobra.Command { PreRunE: func(c *cobra.Command, args []string) error { f := c.Flags() codespaceFlag := f.Lookup("codespace") - configFlag := f.Lookup("config") portFlag := f.Lookup("server-port") profileFlag := f.Lookup("profile") stdioFlag := f.Lookup("stdio") @@ -52,9 +50,6 @@ func newSSHCmd(app *App) *cobra.Command { if !codespaceFlag.Changed { return errors.New("`--stdio` requires explicit `--codespace`") } - if configFlag.Changed { - return errors.New("cannot use `--stdio` with `--config`") - } if portFlag.Changed { return errors.New("cannot use `--stdio` with `--server-port`") } @@ -62,22 +57,10 @@ func newSSHCmd(app *App) *cobra.Command { return errors.New("cannot use `--stdio` with `--profile`") } } - if configFlag.Changed { - if profileFlag.Changed { - return errors.New("cannot use `--config` with `--profile`") - } - if portFlag.Changed { - return errors.New("cannot use `--config` with `--server-port`") - } - } return nil }, RunE: func(cmd *cobra.Command, args []string) error { - if opts.config { - return app.ListOpensshConfig(cmd.Context(), opts) - } else { - return app.SSH(cmd.Context(), args, opts) - } + return app.SSH(cmd.Context(), args, opts) }, DisableFlagsInUseLine: true, } @@ -88,7 +71,8 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") sshCmd.Flags().BoolVarP(&opts.stdio, "stdio", "", false, "Proxy sshd connection to stdio") - sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") + + sshCmd.AddCommand(newConfigCmd(app)) return sshCmd } @@ -177,7 +161,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) ListOpensshConfig(ctx context.Context, opts sshOptions) error { +func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -391,6 +375,37 @@ func (a *App) Copy(ctx context.Context, args []string, opts cpOptions) error { return a.SSH(ctx, nil, opts.sshOptions) } +type configOptions struct { + codespace string +} + +func newConfigCmd(app *App) *cobra.Command { + var opts configOptions + + configCmd := &cobra.Command{ + Use: "config [-c codespace]", + Short: "Write OpenSSH configuration to stdout", + Long: heredoc.Docf(` + The config command generates ssh connection configuration in OpenSSH format. + + If -c/--codespace is specified, configuration is generated for that codespace + only. Otherwise configuration is emitted for all available codespaces. + `, "`"), + Example: heredoc.Doc(` + $ gh codespace config > ~/.ssh/codespaces + $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' + `), + RunE: func(cmd *cobra.Command, args []string) error { + return app.ListOpensshConfig(cmd.Context(), opts) + }, + DisableFlagsInUseLine: true, + } + + configCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") + + return configCmd +} + // fileLogger is a wrapper around an log.Logger configured to write // to a file. It exports two additional methods to get the log file name // and close the file handle when the operation is finished. From 8687fcb2a040bf1992cd8e74f76bd634cd57185f Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 15:18:59 -0700 Subject: [PATCH 017/167] clean up `gh cs ssh` option parsing/validation --- pkg/cmd/codespace/ssh.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index b6c2d6596..031651f80 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -40,20 +40,14 @@ func newSSHCmd(app *App) *cobra.Command { Use: "ssh [...] [-- ...] []", Short: "SSH into a codespace", PreRunE: func(c *cobra.Command, args []string) error { - f := c.Flags() - codespaceFlag := f.Lookup("codespace") - portFlag := f.Lookup("server-port") - profileFlag := f.Lookup("profile") - stdioFlag := f.Lookup("stdio") - - if stdioFlag.Changed { - if !codespaceFlag.Changed { + if opts.stdio { + if opts.codespace == "" { return errors.New("`--stdio` requires explicit `--codespace`") } - if portFlag.Changed { + if opts.serverPort != 0 { return errors.New("cannot use `--stdio` with `--server-port`") } - if profileFlag.Changed { + if opts.profile != "" { return errors.New("cannot use `--stdio` with `--profile`") } } From 3ca5cbab84ece3f415b1d15f9425fe46e7a75c87 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 15:19:13 -0700 Subject: [PATCH 018/167] hide the `gh cs ssh --stdio` option in the --help message --- pkg/cmd/codespace/ssh.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 031651f80..68de85c23 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -64,7 +64,8 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") - sshCmd.Flags().BoolVarP(&opts.stdio, "stdio", "", false, "Proxy sshd connection to stdio") + sshCmd.Flags().BoolVar(&opts.stdio, "stdio", false, "Proxy sshd connection to stdio") + sshCmd.Flags().MarkHidden("stdio") sshCmd.AddCommand(newConfigCmd(app)) From 92609bd1ec29e8b70d4b010d02aea5b9ed264591 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 15:23:36 -0700 Subject: [PATCH 019/167] Revert "use go-multierror to combine read/write close errors" This reverts commit 456f4381e02aa843ddb3cc1b9628cb81487ba895. --- go.mod | 1 - go.sum | 2 -- pkg/cmd/codespace/ssh.go | 13 +++++-------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index e79aff389..fe082e3a3 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( github.com/google/go-cmp v0.5.6 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/gorilla/websocket v1.4.2 - github.com/hashicorp/go-multierror v1.0.0 github.com/hashicorp/go-version v1.3.0 github.com/henvic/httpretty v0.0.6 github.com/itchyny/gojq v0.12.6 diff --git a/go.sum b/go.sum index bd9272641..fd9b7ae9d 100644 --- a/go.sum +++ b/go.sum @@ -192,12 +192,10 @@ github.com/gorilla/websocket v1.4.2/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/ad github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= -github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= -github.com/hashicorp/go-multierror v1.0.0 h1:iVjPR7a6H0tWELX5NxNe7bYopibicUzc7uPribsnS6o= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa6eBIzfwKfwNnHU= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 68de85c23..27fa21149 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -19,7 +19,6 @@ import ( "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/liveshare" - "github.com/hashicorp/go-multierror" "github.com/spf13/cobra" ) @@ -462,12 +461,10 @@ func (crwc *combinedReadWriteCloser) Write(p []byte) (n int, err error) { } func (crwc *combinedReadWriteCloser) Close() error { - var errs error - if err := crwc.writer.Close(); err != nil { - errs = multierror.Append(errs, err) + werr := crwc.writer.Close() + rerr := crwc.reader.Close() + if werr != nil { + return werr } - if err := crwc.reader.Close(); err != nil { - errs = multierror.Append(errs, err) - } - return errs + return rerr } From ac3b0c50e33ac1f327843af3c4a5694372428e29 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:07:26 -0700 Subject: [PATCH 020/167] Ssh -> SSH --- pkg/cmd/codespace/ssh.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 27fa21149..2b74290bd 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -89,7 +89,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e a.errLogger.Printf("Debug file located at: %s", debugLogger.Name()) } - session, err := openSshSession(ctx, a, opts.codespace, liveshareLogger) + session, err := openSSHSession(ctx, a, opts.codespace, liveshareLogger) if err != nil { return fmt.Errorf("error connecting to codespace: %w", err) } @@ -176,7 +176,7 @@ func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { } t, err := template.New("ssh_config").Parse(`Host cs.{{.Name}}.{{.EscapedRef}} - User {{.SshUser}} + User {{.SSHUser}} ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio UserKnownHostsFile=/dev/null StrictHostKeyChecking no @@ -206,7 +206,7 @@ func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { var ok bool if sshUser, ok = sshUsers[cs.Repository.FullName]; !ok { - session, err := openSshSession(ctx, a, cs.Name, nil) + session, err := openSSHSession(ctx, a, cs.Name, nil) if err != nil { fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) continue @@ -223,10 +223,10 @@ func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { sshUsers[cs.Repository.FullName] = sshUser } - conf := codespaceSshConfig{ + conf := codespaceSSHConfig{ Name: cs.Name, EscapedRef: strings.ReplaceAll(cs.GitStatus.Ref, "/", "-"), - SshUser: sshUser, + SSHUser: sshUser, GHExec: ghexec, } if err := t.Execute(a.io.Out, conf); err != nil { @@ -237,14 +237,14 @@ func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { return nil } -type codespaceSshConfig struct { +type codespaceSSHConfig struct { Name string EscapedRef string - SshUser string + SSHUser string GHExec string } -func openSshSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { +func openSSHSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { codespace, err := getOrChooseCodespace(ctx, a.apiClient, csName) if err != nil { return nil, fmt.Errorf("get or choose codespace: %w", err) From ca3b59dd351a1508d7f230a64db3e7240878fec3 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:07:58 -0700 Subject: [PATCH 021/167] use struct embedding to express this less verbosely --- pkg/cmd/codespace/ssh.go | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 2b74290bd..3c729740d 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "io" "io/ioutil" "log" "net" @@ -441,28 +442,17 @@ func (fl *fileLogger) Close() error { } type combinedReadWriteCloser struct { - reader *os.File - writer *os.File + io.ReadCloser + io.WriteCloser } func newCombinedReadWriteCloser(reader *os.File, writer *os.File) (crwc *combinedReadWriteCloser) { - return &combinedReadWriteCloser{ - reader: reader, - writer: writer, - } -} - -func (crwc *combinedReadWriteCloser) Read(p []byte) (n int, err error) { - return crwc.reader.Read(p) -} - -func (crwc *combinedReadWriteCloser) Write(p []byte) (n int, err error) { - return crwc.writer.Write(p) + return &combinedReadWriteCloser{reader, writer} } func (crwc *combinedReadWriteCloser) Close() error { - werr := crwc.writer.Close() - rerr := crwc.reader.Close() + werr := crwc.WriteCloser.Close() + rerr := crwc.ReadCloser.Close() if werr != nil { return werr } From 96a2e125e6b9e64ed48e6423e309ba22ea02e93e Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:37:26 -0700 Subject: [PATCH 022/167] document the codespaceSSHConfig struct --- pkg/cmd/codespace/ssh.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 3c729740d..153cb884c 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -238,11 +238,22 @@ func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { return nil } +// codespaceSSHConfig contains values needed to write an openssh host +// configuration for a single codespace. For example: +// +// Host {{Name}}.{{EscapedRef} +// User {{SSHUser} +// ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio +// +// EscapedRef is included in the name to help distinguish between codespaces +// when tab-completing ssh hostnames. '/' characters in EscapedRef are +// flattened to '-' to prevent problems with tab completion or when the +// hostname appears in ControlMaster socket paths. type codespaceSSHConfig struct { - Name string - EscapedRef string - SSHUser string - GHExec string + Name string // the codespace name, passed to `ssh -c` + EscapedRef string // the currently checked-out branch + SSHUser string // the remote ssh username + GHExec string // path used for invoking the current `gh` binary } func openSSHSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { From 3cb3cf7ca5ec7c10fa5b1e5552e7014b828a7ab6 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:38:47 -0700 Subject: [PATCH 023/167] make ListOpensshConfig private --- pkg/cmd/codespace/ssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 153cb884c..5e9b99ef8 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -156,7 +156,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) ListOpensshConfig(ctx context.Context, opts configOptions) error { +func (a *App) listOpensshConfig(ctx context.Context, opts configOptions) error { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -402,7 +402,7 @@ func newConfigCmd(app *App) *cobra.Command { $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' `), RunE: func(cmd *cobra.Command, args []string) error { - return app.ListOpensshConfig(cmd.Context(), opts) + return app.listOpensshConfig(cmd.Context(), opts) }, DisableFlagsInUseLine: true, } From b7cd2bbc72464e9e1ed87853ef2c9046c9ed5032 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:39:42 -0700 Subject: [PATCH 024/167] no need to separately declare vars here --- pkg/cmd/codespace/ssh.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 5e9b99ef8..abc95f318 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -203,10 +203,8 @@ func (a *App) listOpensshConfig(ctx context.Context, opts configOptions) error { continue } - var sshUser string - var ok bool - - if sshUser, ok = sshUsers[cs.Repository.FullName]; !ok { + sshUser, ok := sshUsers[cs.Repository.FullName] + if !ok { session, err := openSSHSession(ctx, a, cs.Name, nil) if err != nil { fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) From 6f8635a17e69e879ea730e32296a719a1c0c2f58 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:43:55 -0700 Subject: [PATCH 025/167] listOpensshConfig -> printOpenSSHConfig --- pkg/cmd/codespace/ssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index abc95f318..301f9b49e 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -156,7 +156,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) listOpensshConfig(ctx context.Context, opts configOptions) error { +func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -400,7 +400,7 @@ func newConfigCmd(app *App) *cobra.Command { $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' `), RunE: func(cmd *cobra.Command, args []string) error { - return app.listOpensshConfig(cmd.Context(), opts) + return app.printOpenSSHConfig(cmd.Context(), opts) }, DisableFlagsInUseLine: true, } From 4a6887f99e0be2df3deb5e94656ca649c11cb838 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:47:16 -0700 Subject: [PATCH 026/167] add a comment explaining why we build an sshUsers map --- pkg/cmd/codespace/ssh.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 301f9b49e..7dfce6ec4 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -194,6 +194,10 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return err } + // store a mapping of repository -> remote ssh username. This is + // necessary because the username can vary between codespaces, but + // since fetching it is slow, we store it here so we at least only do + // it once per repository. sshUsers := map[string]string{} for _, cs := range codespaces { From 369ea534b67add703502849909db3c7f2767e277 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:52:07 -0700 Subject: [PATCH 027/167] explain why failure to connect to a codespace doesn't fail the entire command --- pkg/cmd/codespace/ssh.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 7dfce6ec4..9e35bc544 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -212,6 +212,10 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error session, err := openSSHSession(ctx, a, cs.Name, nil) if err != nil { fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) + + // Move on to the next codespace. We don't want to bail here - just because we're not + // able to set up connectivity to one doesn't mean we shouldn't make a best effort to + // generate configs for the rest of them. continue } defer session.Close() @@ -221,7 +225,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error a.StopProgressIndicator() if err != nil { fmt.Fprintf(os.Stderr, "error getting ssh server details: %v", err) - continue + continue // see above } sshUsers[cs.Repository.FullName] = sshUser } From 71c9fa11f06034ceaa9dce3971f6942e6c94ebb2 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:57:51 -0700 Subject: [PATCH 028/167] add more `ssh config` --help documentation --- pkg/cmd/codespace/ssh.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 9e35bc544..bfa7e0391 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -398,10 +398,17 @@ func newConfigCmd(app *App) *cobra.Command { Use: "config [-c codespace]", Short: "Write OpenSSH configuration to stdout", Long: heredoc.Docf(` - The config command generates ssh connection configuration in OpenSSH format. + The config command generates per-codespace ssh configuration in OpenSSH format. + + Including this configuration in ~/.ssh/config simplifies integration with other tools + that integrate with OpenSSH, such as bash/zsh ssh hostname completion, remote path + completion for scp/rsync/sshfs, git ssh remotes, and so on. If -c/--codespace is specified, configuration is generated for that codespace only. Otherwise configuration is emitted for all available codespaces. + + Codespaces that aren't in "Available" state are skipped because it's necessary to + connect to the running codespace to determine the required remote ssh username. `, "`"), Example: heredoc.Doc(` $ gh codespace config > ~/.ssh/codespaces From ac685611b117451163f1df2874433b8eef35ed62 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 16:58:16 -0700 Subject: [PATCH 029/167] openssh -> OpenSSH --- pkg/cmd/codespace/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index bfa7e0391..5654f3432 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -244,7 +244,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return nil } -// codespaceSSHConfig contains values needed to write an openssh host +// codespaceSSHConfig contains values needed to write an OpenSSH host // configuration for a single codespace. For example: // // Host {{Name}}.{{EscapedRef} From a9d02a746baa3b3cfa17d7c9cd1f80e030cb4f65 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 16 Dec 2021 17:19:34 -0700 Subject: [PATCH 030/167] push fetching codespace details into openSSHSession callers --- pkg/cmd/codespace/ssh.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 5654f3432..33f837bbc 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -90,7 +90,12 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e a.errLogger.Printf("Debug file located at: %s", debugLogger.Name()) } - session, err := openSSHSession(ctx, a, opts.codespace, liveshareLogger) + codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + if err != nil { + return fmt.Errorf("get or choose codespace: %w", err) + } + + session, err := openSSHSession(ctx, a, codespace, liveshareLogger) if err != nil { return fmt.Errorf("error connecting to codespace: %w", err) } @@ -209,7 +214,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error sshUser, ok := sshUsers[cs.Repository.FullName] if !ok { - session, err := openSSHSession(ctx, a, cs.Name, nil) + codespace, err := a.apiClient.GetCodespace(ctx, cs.Name, true) + if err != nil { + return fmt.Errorf("getting full codespace details: %w", err) + } + + session, err := openSSHSession(ctx, a, codespace, nil) if err != nil { fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) @@ -262,12 +272,7 @@ type codespaceSSHConfig struct { GHExec string // path used for invoking the current `gh` binary } -func openSSHSession(ctx context.Context, a *App, csName string, liveshareLogger *log.Logger) (*liveshare.Session, error) { - codespace, err := getOrChooseCodespace(ctx, a.apiClient, csName) - if err != nil { - return nil, fmt.Errorf("get or choose codespace: %w", err) - } - +func openSSHSession(ctx context.Context, a *App, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { // TODO(josebalius): We can fetch the user in parallel to everything else // we should convert this call and others to happen async user, err := a.apiClient.GetUser(ctx) From 206b6379c32d85b29768eff3fffea013c590995b Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 10:47:22 -0700 Subject: [PATCH 031/167] return non-zero if `ssh config` skips any codespaces --- pkg/cmd/codespace/ssh.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 33f837bbc..eb4bae7a2 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -205,10 +205,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error // it once per repository. sshUsers := map[string]string{} + var status error for _, cs := range codespaces { if cs.State != "Available" { fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) + status = cmdutil.SilentError continue } @@ -226,6 +228,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error // Move on to the next codespace. We don't want to bail here - just because we're not // able to set up connectivity to one doesn't mean we shouldn't make a best effort to // generate configs for the rest of them. + status = cmdutil.SilentError continue } defer session.Close() @@ -235,6 +238,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error a.StopProgressIndicator() if err != nil { fmt.Fprintf(os.Stderr, "error getting ssh server details: %v", err) + status = cmdutil.SilentError continue // see above } sshUsers[cs.Repository.FullName] = sshUser @@ -251,7 +255,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error } } - return nil + return status } // codespaceSSHConfig contains values needed to write an OpenSSH host From 811d6505d28c82e4c03b2778be8a2a71794fe888 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 11:03:11 -0700 Subject: [PATCH 032/167] tighten up `ssh config` help wording --- pkg/cmd/codespace/ssh.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index eb4bae7a2..510a65502 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -409,9 +409,9 @@ func newConfigCmd(app *App) *cobra.Command { Long: heredoc.Docf(` The config command generates per-codespace ssh configuration in OpenSSH format. - Including this configuration in ~/.ssh/config simplifies integration with other tools - that integrate with OpenSSH, such as bash/zsh ssh hostname completion, remote path - completion for scp/rsync/sshfs, git ssh remotes, and so on. + Including this configuration in ~/.ssh/config improves the user experience of other + tools that integrate with OpenSSH, such as bash/zsh completion of ssh hostnames, + remote path completion for scp/rsync/sshfs, git ssh remotes, and so on. If -c/--codespace is specified, configuration is generated for that codespace only. Otherwise configuration is emitted for all available codespaces. @@ -429,7 +429,7 @@ func newConfigCmd(app *App) *cobra.Command { DisableFlagsInUseLine: true, } - configCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") + configCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of a codespace") return configCmd } From 0670a7758b1eb17f91e10480fe7a61143f91afba Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 11:18:59 -0700 Subject: [PATCH 033/167] use generic io interfaces --- pkg/cmd/codespace/ssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 510a65502..24729ecca 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -110,7 +110,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e if opts.stdio { fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) - stdio := newCombinedReadWriteCloser(os.Stdin, os.Stdout) + stdio := newReadWriteCloser(os.Stdin, os.Stdout) err := fwd.Forward(ctx, stdio) // always non-nil return fmt.Errorf("tunnel closed: %w", err) } @@ -479,7 +479,7 @@ type combinedReadWriteCloser struct { io.WriteCloser } -func newCombinedReadWriteCloser(reader *os.File, writer *os.File) (crwc *combinedReadWriteCloser) { +func newReadWriteCloser(reader io.ReadCloser, writer io.WriteCloser) io.ReadWriteCloser { return &combinedReadWriteCloser{reader, writer} } From a05541f4ed924e9732144959790405bb1a63b39e Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 11:39:14 -0700 Subject: [PATCH 034/167] drop redundant API call --- pkg/cmd/codespace/ssh.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 24729ecca..8275b0634 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -216,12 +216,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error sshUser, ok := sshUsers[cs.Repository.FullName] if !ok { - codespace, err := a.apiClient.GetCodespace(ctx, cs.Name, true) - if err != nil { - return fmt.Errorf("getting full codespace details: %w", err) - } - - session, err := openSSHSession(ctx, a, codespace, nil) + session, err := openSSHSession(ctx, a, cs, nil) if err != nil { fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) From 2ee88da647c85f957de710519ed628ce59a14687 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 14:42:54 -0700 Subject: [PATCH 035/167] close session on error --- pkg/cmd/codespace/ssh.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 8275b0634..dfca3ec03 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -294,6 +294,7 @@ func openSSHSession(ctx context.Context, a *App, codespace *api.Codespace, lives } if err := <-authkeys; err != nil { + session.Close() return nil, err } From 06eb5ad47fbb055046bb3c86ee12587d7a714c93 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 16:10:44 -0700 Subject: [PATCH 036/167] fetch remote ssh usernames in parallel --- pkg/cmd/codespace/ssh.go | 103 ++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 45 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index dfca3ec03..72b5611d5 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -162,7 +162,6 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error { - // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -181,6 +180,51 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return fmt.Errorf("error getting codespace info: %w", err) } + sshUsers := make(chan sshResult) + fetches := 0 + var status error + for _, cs := range codespaces { + if cs.State != "Available" { + fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) + status = cmdutil.SilentError + continue + } + + cs := cs + fetches += 1 + go func() { + result := sshResult{} + defer func() { + select { + case sshUsers <- result: + case <-ctx.Done(): + } + }() + + session, err := openSSHSession(ctx, a, cs, nil) + if err != nil { + result.err = fmt.Errorf("error connecting to codespace: %w", err) + return + } + defer session.Close() + + //a.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching SSH Details for %s", cs.Name)) + _, result.user, err = session.StartSSHServer(ctx) + //a.StopProgressIndicator() + if err != nil { + result.err = fmt.Errorf("error getting ssh server details: %w", err) + return + } + + result.codespace = cs + }() + } + + ghexec, err := os.Executable() + if err != nil { + return err + } + t, err := template.New("ssh_config").Parse(`Host cs.{{.Name}}.{{.EscapedRef}} User {{.SSHUser}} ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio @@ -194,55 +238,18 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return fmt.Errorf("error formatting template: %w", err) } - ghexec, err := os.Executable() - if err != nil { - return err - } - - // store a mapping of repository -> remote ssh username. This is - // necessary because the username can vary between codespaces, but - // since fetching it is slow, we store it here so we at least only do - // it once per repository. - sshUsers := map[string]string{} - - var status error - for _, cs := range codespaces { - - if cs.State != "Available" { - fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) + for i := 0; i < fetches; i++ { + result := <-sshUsers + if result.err != nil { + fmt.Fprintf(os.Stderr, "%v\n", result.err) status = cmdutil.SilentError continue } - sshUser, ok := sshUsers[cs.Repository.FullName] - if !ok { - session, err := openSSHSession(ctx, a, cs, nil) - if err != nil { - fmt.Fprintf(os.Stderr, "error connecting to codespace: %v\n", err) - - // Move on to the next codespace. We don't want to bail here - just because we're not - // able to set up connectivity to one doesn't mean we shouldn't make a best effort to - // generate configs for the rest of them. - status = cmdutil.SilentError - continue - } - defer session.Close() - - a.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching SSH Details for %s", cs.Name)) - _, sshUser, err = session.StartSSHServer(ctx) - a.StopProgressIndicator() - if err != nil { - fmt.Fprintf(os.Stderr, "error getting ssh server details: %v", err) - status = cmdutil.SilentError - continue // see above - } - sshUsers[cs.Repository.FullName] = sshUser - } - conf := codespaceSSHConfig{ - Name: cs.Name, - EscapedRef: strings.ReplaceAll(cs.GitStatus.Ref, "/", "-"), - SSHUser: sshUser, + Name: result.codespace.Name, + EscapedRef: strings.ReplaceAll(result.codespace.GitStatus.Ref, "/", "-"), + SSHUser: result.user, GHExec: ghexec, } if err := t.Execute(a.io.Out, conf); err != nil { @@ -253,6 +260,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return status } +type sshResult struct { + codespace *api.Codespace + user string // on success, the remote ssh username; else nil + err error +} + // codespaceSSHConfig contains values needed to write an OpenSSH host // configuration for a single codespace. For example: // From 0af268da4edb6b67f29704391ed0f4fdf8aabdbc Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 16:11:55 -0700 Subject: [PATCH 037/167] properly indent ssh config example --- pkg/cmd/codespace/ssh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 72b5611d5..36aef65fd 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -270,8 +270,8 @@ type sshResult struct { // configuration for a single codespace. For example: // // Host {{Name}}.{{EscapedRef} -// User {{SSHUser} -// ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio +// User {{SSHUser} +// ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio // // EscapedRef is included in the name to help distinguish between codespaces // when tab-completing ssh hostnames. '/' characters in EscapedRef are From 61823997c228ecf8b52e2c1c9fcfdff8f1589549 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 16:46:53 -0700 Subject: [PATCH 038/167] always verify authorized keys in parallel with other work, and at most once --- pkg/cmd/codespace/ssh.go | 46 +++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 36aef65fd..5dc4050a6 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -95,8 +95,18 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e return fmt.Errorf("get or choose codespace: %w", err) } - session, err := openSSHSession(ctx, a, codespace, liveshareLogger) + // While connecting, ensure in the background that the user has keys installed. + // That lets us report a more useful error message if they don't. + authkeys := make(chan error, 1) + go func() { + authkeys <- a.ensureAuthorizedKeys(ctx) + }() + + session, err := a.openSSHSession(ctx, codespace, liveshareLogger) if err != nil { + if authErr := <-authkeys; authErr != nil { + return authErr + } return fmt.Errorf("error connecting to codespace: %w", err) } defer safeClose(session, &err) @@ -201,7 +211,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error } }() - session, err := openSSHSession(ctx, a, cs, nil) + session, err := a.openSSHSession(ctx, cs, nil) if err != nil { result.err = fmt.Errorf("error connecting to codespace: %w", err) return @@ -220,6 +230,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error }() } + // While the above fetches are running, ensure that the user has keys installed. + // That lets us report a more useful error message if they don't. + if err = a.ensureAuthorizedKeys(ctx); err != nil { + return err + } + ghexec, err := os.Executable() if err != nil { return err @@ -284,19 +300,7 @@ type codespaceSSHConfig struct { GHExec string // path used for invoking the current `gh` binary } -func openSSHSession(ctx context.Context, a *App, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { - // TODO(josebalius): We can fetch the user in parallel to everything else - // we should convert this call and others to happen async - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return nil, fmt.Errorf("error getting user: %w", err) - } - - authkeys := make(chan error, 1) - go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) - }() - +func (a *App) openSSHSession(ctx context.Context, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { if liveshareLogger == nil { liveshareLogger = noopLogger() } @@ -306,12 +310,16 @@ func openSSHSession(ctx context.Context, a *App, codespace *api.Codespace, lives return nil, fmt.Errorf("error connecting to codespace: %w", err) } - if err := <-authkeys; err != nil { - session.Close() - return nil, err + return session, nil +} + +func (a *App) ensureAuthorizedKeys(ctx context.Context) error { + user, err := a.apiClient.GetUser(ctx) + if err != nil { + return fmt.Errorf("error getting user: %w", err) } - return session, nil + return checkAuthorizedKeys(ctx, a.apiClient, user.Login) } type cpOptions struct { From b2598d64f97a59558e06cd58ea21ebdf50e78e9d Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 16:57:29 -0700 Subject: [PATCH 039/167] start codespace to fetch config if it's explicitly requested When running `gh cs ssh config` without a `-c` option, we skip codespaces that aren't available. This change suppresses that behavior when a single codespace is explicitly requested, starting the codespace if it's not running. --- pkg/cmd/codespace/ssh.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 5dc4050a6..6349db5fc 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -194,7 +194,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error fetches := 0 var status error for _, cs := range codespaces { - if cs.State != "Available" { + if cs.State != "Available" && opts.codespace == "" { fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) status = cmdutil.SilentError continue @@ -423,7 +423,7 @@ func newConfigCmd(app *App) *cobra.Command { configCmd := &cobra.Command{ Use: "config [-c codespace]", Short: "Write OpenSSH configuration to stdout", - Long: heredoc.Docf(` + Long: heredoc.Doc(` The config command generates per-codespace ssh configuration in OpenSSH format. Including this configuration in ~/.ssh/config improves the user experience of other @@ -433,9 +433,10 @@ func newConfigCmd(app *App) *cobra.Command { If -c/--codespace is specified, configuration is generated for that codespace only. Otherwise configuration is emitted for all available codespaces. - Codespaces that aren't in "Available" state are skipped because it's necessary to - connect to the running codespace to determine the required remote ssh username. - `, "`"), + When generating configuration for all codespaces, ones that aren't in "Available" + state are skipped because it's necessary to start the codespace to determine its + remote ssh username. When generating configuration for a single codespace with '-c', + `), Example: heredoc.Doc(` $ gh codespace config > ~/.ssh/codespaces $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' From 7b432de5c2af8862350f40a7a89fd4fd32f6ed27 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Fri, 17 Dec 2021 17:09:06 -0700 Subject: [PATCH 040/167] use heredoc helper for config template --- pkg/cmd/codespace/ssh.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 6349db5fc..86ed5d068 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -241,15 +241,16 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return err } - t, err := template.New("ssh_config").Parse(`Host cs.{{.Name}}.{{.EscapedRef}} - User {{.SSHUser}} - ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio - UserKnownHostsFile=/dev/null - StrictHostKeyChecking no - LogLevel quiet - ControlMaster auto + t, err := template.New("ssh_config").Parse(heredoc.Doc(` + Host cs.{{.Name}}.{{.EscapedRef}} + User {{.SSHUser}} + ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio + UserKnownHostsFile=/dev/null + StrictHostKeyChecking no + LogLevel quiet + ControlMaster auto -`) + `)) if err != nil { return fmt.Errorf("error formatting template: %w", err) } From f22be4a03d489c35f3f57ce96a5449ebc3ad4249 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:27:42 -0700 Subject: [PATCH 041/167] use a more robust method to get the executable path cmdutil.Factory.Executable() accounts for things like package managers and symlinks to the actual executable. An alternative to passing the *cmdutil.Factory down the stack would be stashing the executable string in the codespace.App, which works (and the diff is smaller), but it produced some odd non-local test failures. This way seems less mysterious and more like other uses of Factory in the codebase. --- pkg/cmd/codespace/root.go | 5 +++-- pkg/cmd/codespace/ssh.go | 17 ++++++----------- pkg/cmd/root/root.go | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/codespace/root.go b/pkg/cmd/codespace/root.go index 5b2c0d8fc..5a3499dee 100644 --- a/pkg/cmd/codespace/root.go +++ b/pkg/cmd/codespace/root.go @@ -1,10 +1,11 @@ package codespace import ( + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) -func NewRootCmd(app *App) *cobra.Command { +func NewRootCmd(app *App, f *cmdutil.Factory) *cobra.Command { root := &cobra.Command{ Use: "codespace", Short: "Connect to and manage your codespaces", @@ -16,7 +17,7 @@ func NewRootCmd(app *App) *cobra.Command { root.AddCommand(newListCmd(app)) root.AddCommand(newLogsCmd(app)) root.AddCommand(newPortsCmd(app)) - root.AddCommand(newSSHCmd(app)) + root.AddCommand(newSSHCmd(app, f)) root.AddCommand(newCpCmd(app)) root.AddCommand(newStopCmd(app)) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 86ed5d068..be19eb3f9 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -33,7 +33,7 @@ type sshOptions struct { scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } -func newSSHCmd(app *App) *cobra.Command { +func newSSHCmd(app *App, f *cmdutil.Factory) *cobra.Command { var opts sshOptions sshCmd := &cobra.Command{ @@ -67,7 +67,7 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().BoolVar(&opts.stdio, "stdio", false, "Proxy sshd connection to stdio") sshCmd.Flags().MarkHidden("stdio") - sshCmd.AddCommand(newConfigCmd(app)) + sshCmd.AddCommand(newConfigCmd(app, f)) return sshCmd } @@ -171,7 +171,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error { +func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, executable string) error { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -236,11 +236,6 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error return err } - ghexec, err := os.Executable() - if err != nil { - return err - } - t, err := template.New("ssh_config").Parse(heredoc.Doc(` Host cs.{{.Name}}.{{.EscapedRef}} User {{.SSHUser}} @@ -267,7 +262,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error Name: result.codespace.Name, EscapedRef: strings.ReplaceAll(result.codespace.GitStatus.Ref, "/", "-"), SSHUser: result.user, - GHExec: ghexec, + GHExec: executable, } if err := t.Execute(a.io.Out, conf); err != nil { return err @@ -418,7 +413,7 @@ type configOptions struct { codespace string } -func newConfigCmd(app *App) *cobra.Command { +func newConfigCmd(app *App, f *cmdutil.Factory) *cobra.Command { var opts configOptions configCmd := &cobra.Command{ @@ -443,7 +438,7 @@ func newConfigCmd(app *App) *cobra.Command { $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' `), RunE: func(cmd *cobra.Command, args []string) error { - return app.printOpenSSHConfig(cmd.Context(), opts) + return app.printOpenSSHConfig(cmd.Context(), opts, f.Executable()) }, DisableFlagsInUseLine: true, } diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index d393d1a7f..58272306c 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -142,7 +142,7 @@ func newCodespaceCmd(f *cmdutil.Factory) *cobra.Command { &lazyLoadedHTTPClient{factory: f}, ), ) - cmd := codespaceCmd.NewRootCmd(app) + cmd := codespaceCmd.NewRootCmd(app, f) cmd.Use = "codespace" cmd.Aliases = []string{"cs"} cmd.Annotations = map[string]string{"IsCore": "true"} From 6b34fa2a274dda76349d3fab84c11297a2b40446 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:38:51 -0700 Subject: [PATCH 042/167] oh look, struct definitions can be scoped! --- pkg/cmd/codespace/ssh.go | 48 ++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index be19eb3f9..66dae59d9 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -190,6 +190,12 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut return fmt.Errorf("error getting codespace info: %w", err) } + type sshResult struct { + codespace *api.Codespace + user string // on success, the remote ssh username; else nil + err error + } + sshUsers := make(chan sshResult) fetches := 0 var status error @@ -258,6 +264,24 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut continue } + // codespaceSSHConfig contains values needed to write an OpenSSH host + // configuration for a single codespace. For example: + // + // Host {{Name}}.{{EscapedRef} + // User {{SSHUser} + // ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio + // + // EscapedRef is included in the name to help distinguish between codespaces + // when tab-completing ssh hostnames. '/' characters in EscapedRef are + // flattened to '-' to prevent problems with tab completion or when the + // hostname appears in ControlMaster socket paths. + type codespaceSSHConfig struct { + Name string // the codespace name, passed to `ssh -c` + EscapedRef string // the currently checked-out branch + SSHUser string // the remote ssh username + GHExec string // path used for invoking the current `gh` binary + } + conf := codespaceSSHConfig{ Name: result.codespace.Name, EscapedRef: strings.ReplaceAll(result.codespace.GitStatus.Ref, "/", "-"), @@ -272,30 +296,6 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut return status } -type sshResult struct { - codespace *api.Codespace - user string // on success, the remote ssh username; else nil - err error -} - -// codespaceSSHConfig contains values needed to write an OpenSSH host -// configuration for a single codespace. For example: -// -// Host {{Name}}.{{EscapedRef} -// User {{SSHUser} -// ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio -// -// EscapedRef is included in the name to help distinguish between codespaces -// when tab-completing ssh hostnames. '/' characters in EscapedRef are -// flattened to '-' to prevent problems with tab completion or when the -// hostname appears in ControlMaster socket paths. -type codespaceSSHConfig struct { - Name string // the codespace name, passed to `ssh -c` - EscapedRef string // the currently checked-out branch - SSHUser string // the remote ssh username - GHExec string // path used for invoking the current `gh` binary -} - func (a *App) openSSHSession(ctx context.Context, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { if liveshareLogger == nil { liveshareLogger = noopLogger() From ae3aacb9641c630fa4e4edcb1fb8742083472d69 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:43:55 -0700 Subject: [PATCH 043/167] fix `errcheck` linter warning --- pkg/cmd/codespace/ssh.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 66dae59d9..e16bcb2a0 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -65,7 +65,9 @@ func newSSHCmd(app *App, f *cmdutil.Factory) *cobra.Command { sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") sshCmd.Flags().BoolVar(&opts.stdio, "stdio", false, "Proxy sshd connection to stdio") - sshCmd.Flags().MarkHidden("stdio") + if err := sshCmd.Flags().MarkHidden("stdio"); err != nil { + fmt.Fprintf(os.Stderr, "%v\n", err) + } sshCmd.AddCommand(newConfigCmd(app, f)) From 37f8039f76f112dc9ca827f0e2fa48e22e84dbb8 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:49:58 -0700 Subject: [PATCH 044/167] merge ensureAuthorizedKeys into checkAuthorizedKeys --- pkg/cmd/codespace/common.go | 9 +++++++-- pkg/cmd/codespace/logs.go | 7 +------ pkg/cmd/codespace/ssh.go | 13 ++----------- 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 0f6f9cf4e..09b9308aa 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -209,8 +209,13 @@ func ask(qs []*survey.Question, response interface{}) error { // checkAuthorizedKeys reports an error if the user has not registered any SSH keys; // see https://github.com/cli/cli/v2/issues/166#issuecomment-921769703. // The check is not required for security but it improves the error message. -func checkAuthorizedKeys(ctx context.Context, client apiClient, user string) error { - keys, err := client.AuthorizedKeys(ctx, user) +func checkAuthorizedKeys(ctx context.Context, client apiClient) error { + user, err := client.GetUser(ctx) + if err != nil { + return fmt.Errorf("error getting user: %w", err) + } + + keys, err := client.AuthorizedKeys(ctx, user.Login) if err != nil { return fmt.Errorf("failed to read GitHub-authorized SSH keys for %s: %w", user, err) } diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index 9fc6010b4..d0a0c233b 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -41,14 +41,9 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err return fmt.Errorf("get or choose codespace: %w", err) } - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return fmt.Errorf("getting user: %w", err) - } - authkeys := make(chan error, 1) go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) + authkeys <- checkAuthorizedKeys(ctx, a.apiClient) }() session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, codespace) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index e16bcb2a0..cce05f131 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -101,7 +101,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e // That lets us report a more useful error message if they don't. authkeys := make(chan error, 1) go func() { - authkeys <- a.ensureAuthorizedKeys(ctx) + authkeys <- checkAuthorizedKeys(ctx, a.apiClient) }() session, err := a.openSSHSession(ctx, codespace, liveshareLogger) @@ -240,7 +240,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut // While the above fetches are running, ensure that the user has keys installed. // That lets us report a more useful error message if they don't. - if err = a.ensureAuthorizedKeys(ctx); err != nil { + if err = checkAuthorizedKeys(ctx, a.apiClient); err != nil { return err } @@ -311,15 +311,6 @@ func (a *App) openSSHSession(ctx context.Context, codespace *api.Codespace, live return session, nil } -func (a *App) ensureAuthorizedKeys(ctx context.Context) error { - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return fmt.Errorf("error getting user: %w", err) - } - - return checkAuthorizedKeys(ctx, a.apiClient, user.Login) -} - type cpOptions struct { sshOptions recursive bool // -r From 28dd73ffdf1d5defab21e4b46d21a60b58fbc28c Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:52:39 -0700 Subject: [PATCH 045/167] always pass a non-nil logger to openSSHSession --- pkg/cmd/codespace/ssh.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index cce05f131..ce7e7be1d 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -219,7 +219,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut } }() - session, err := a.openSSHSession(ctx, cs, nil) + session, err := a.openSSHSession(ctx, cs, noopLogger()) if err != nil { result.err = fmt.Errorf("error connecting to codespace: %w", err) return @@ -299,10 +299,6 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut } func (a *App) openSSHSession(ctx context.Context, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { - if liveshareLogger == nil { - liveshareLogger = noopLogger() - } - session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) if err != nil { return nil, fmt.Errorf("error connecting to codespace: %w", err) From 81b34d272ce262495b8fd3446763310fa7c55d63 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 11:58:52 -0700 Subject: [PATCH 046/167] inline openSSHSession --- pkg/cmd/codespace/ssh.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index ce7e7be1d..af1c3cc06 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -104,7 +104,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e authkeys <- checkAuthorizedKeys(ctx, a.apiClient) }() - session, err := a.openSSHSession(ctx, codespace, liveshareLogger) + session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) if err != nil { if authErr := <-authkeys; authErr != nil { return authErr @@ -178,15 +178,15 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut defer cancel() var err error - var codespaces []*api.Codespace + var csList []*api.Codespace if opts.codespace == "" { a.StartProgressIndicatorWithLabel("Fetching codespaces") - codespaces, err = a.apiClient.ListCodespaces(ctx, -1) + csList, err = a.apiClient.ListCodespaces(ctx, -1) a.StopProgressIndicator() } else { var codespace *api.Codespace codespace, err = getOrChooseCodespace(ctx, a.apiClient, opts.codespace) - codespaces = []*api.Codespace{codespace} + csList = []*api.Codespace{codespace} } if err != nil { return fmt.Errorf("error getting codespace info: %w", err) @@ -201,7 +201,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut sshUsers := make(chan sshResult) fetches := 0 var status error - for _, cs := range codespaces { + for _, cs := range csList { if cs.State != "Available" && opts.codespace == "" { fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) status = cmdutil.SilentError @@ -219,7 +219,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut } }() - session, err := a.openSSHSession(ctx, cs, noopLogger()) + session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, cs) if err != nil { result.err = fmt.Errorf("error connecting to codespace: %w", err) return @@ -298,15 +298,6 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut return status } -func (a *App) openSSHSession(ctx context.Context, codespace *api.Codespace, liveshareLogger *log.Logger) (*liveshare.Session, error) { - session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) - if err != nil { - return nil, fmt.Errorf("error connecting to codespace: %w", err) - } - - return session, nil -} - type cpOptions struct { sshOptions recursive bool // -r From a864985f0a007de2f880f4e3a8ca066251dcb2ca Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 12:12:22 -0700 Subject: [PATCH 047/167] use WaitGroup for a more idiomatic concurrency pattern --- pkg/cmd/codespace/ssh.go | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index af1c3cc06..df19c9c83 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "sync" "text/template" "github.com/MakeNowJust/heredoc" @@ -199,7 +200,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut } sshUsers := make(chan sshResult) - fetches := 0 + var wg sync.WaitGroup var status error for _, cs := range csList { if cs.State != "Available" && opts.codespace == "" { @@ -209,35 +210,34 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut } cs := cs - fetches += 1 + wg.Add(1) go func() { result := sshResult{} - defer func() { - select { - case sshUsers <- result: - case <-ctx.Done(): - } - }() + defer wg.Done() session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, cs) if err != nil { result.err = fmt.Errorf("error connecting to codespace: %w", err) - return - } - defer session.Close() + } else { + defer session.Close() - //a.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching SSH Details for %s", cs.Name)) - _, result.user, err = session.StartSSHServer(ctx) - //a.StopProgressIndicator() - if err != nil { - result.err = fmt.Errorf("error getting ssh server details: %w", err) - return + _, result.user, err = session.StartSSHServer(ctx) + if err != nil { + result.err = fmt.Errorf("error getting ssh server details: %w", err) + } else { + result.codespace = cs + } } - result.codespace = cs + sshUsers <- result }() } + go func() { + wg.Wait() + close(sshUsers) + }() + // While the above fetches are running, ensure that the user has keys installed. // That lets us report a more useful error message if they don't. if err = checkAuthorizedKeys(ctx, a.apiClient); err != nil { @@ -258,8 +258,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut return fmt.Errorf("error formatting template: %w", err) } - for i := 0; i < fetches; i++ { - result := <-sshUsers + for result := range sshUsers { if result.err != nil { fmt.Fprintf(os.Stderr, "%v\n", result.err) status = cmdutil.SilentError From 7bd6fe9af8f4c139546c213d3521f2cbc6f82972 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 12:28:25 -0700 Subject: [PATCH 048/167] maximize the time checkAuthorizedKeys has to run concurrently Also, other than that, restore the original ordering of this function --- pkg/cmd/codespace/ssh.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index df19c9c83..2c214668f 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -81,6 +81,18 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e ctx, cancel := context.WithCancel(ctx) defer cancel() + // While connecting, ensure in the background that the user has keys installed. + // That lets us report a more useful error message if they don't. + authkeys := make(chan error, 1) + go func() { + authkeys <- checkAuthorizedKeys(ctx, a.apiClient) + }() + + codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + if err != nil { + return fmt.Errorf("get or choose codespace: %w", err) + } + liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) @@ -93,18 +105,6 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e a.errLogger.Printf("Debug file located at: %s", debugLogger.Name()) } - codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) - if err != nil { - return fmt.Errorf("get or choose codespace: %w", err) - } - - // While connecting, ensure in the background that the user has keys installed. - // That lets us report a more useful error message if they don't. - authkeys := make(chan error, 1) - go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient) - }() - session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) if err != nil { if authErr := <-authkeys; authErr != nil { From 932c9da4736089180083cccc4e830512eb7982a7 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 12:39:00 -0700 Subject: [PATCH 049/167] clean up inadvertently truncated help message --- pkg/cmd/codespace/ssh.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 2c214668f..d8ee955f1 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -401,16 +401,17 @@ func newConfigCmd(app *App, f *cmdutil.Factory) *cobra.Command { Long: heredoc.Doc(` The config command generates per-codespace ssh configuration in OpenSSH format. - Including this configuration in ~/.ssh/config improves the user experience of other + Including this configuration in ~/.ssh/config improves the user experience of tools that integrate with OpenSSH, such as bash/zsh completion of ssh hostnames, remote path completion for scp/rsync/sshfs, git ssh remotes, and so on. If -c/--codespace is specified, configuration is generated for that codespace only. Otherwise configuration is emitted for all available codespaces. - When generating configuration for all codespaces, ones that aren't in "Available" - state are skipped because it's necessary to start the codespace to determine its - remote ssh username. When generating configuration for a single codespace with '-c', + When generating configuration for all codespaces, ones that aren't in + "Available" state are skipped because it's necessary to start the codespace to + determine its remote ssh username. However, when using '-c' to generate + configuration for a single codespace, it will be started if necessary. `), Example: heredoc.Doc(` $ gh codespace config > ~/.ssh/codespaces From 38eb894d73132dfc3d3c8ec57d42f57a49cef54b Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Mon, 20 Dec 2021 13:33:13 -0700 Subject: [PATCH 050/167] prevent leaking any blocked goroutines on error --- pkg/cmd/codespace/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index d8ee955f1..18730a176 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -199,7 +199,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut err error } - sshUsers := make(chan sshResult) + sshUsers := make(chan sshResult, len(csList)) var wg sync.WaitGroup var status error for _, cs := range csList { From 3cce16e72d76b2c3981fe27ff8a8864e565b2a5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Dec 2021 13:50:55 +0100 Subject: [PATCH 051/167] Refactor `factory.Executable()` to be a method rather than a func This way, factory can satisfy an interface that requires `Executable()`. --- pkg/cmd/auth/login/login_test.go | 3 +- pkg/cmd/auth/refresh/refresh_test.go | 3 +- pkg/cmd/factory/default.go | 60 ++------------------------- pkg/cmdutil/factory.go | 61 ++++++++++++++++++++++++++-- 4 files changed, 63 insertions(+), 64 deletions(-) diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index f6fbcabd5..c3cde03cf 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -147,8 +147,7 @@ func Test_NewCmdLogin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, stdin, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdoutTTY(true) diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index dbdae26af..c8dbebbe5 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -91,8 +91,7 @@ func Test_NewCmdRefresh(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, _, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdinTTY(tt.tty) io.SetStdoutTTY(tt.tty) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 08d93c2be..110dfb4e9 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "os" - "path/filepath" "time" "github.com/cli/cli/v2/api" @@ -19,17 +18,10 @@ import ( ) func New(appVersion string) *cmdutil.Factory { - var exe string f := &cmdutil.Factory{ - Config: configFunc(), // No factory dependencies - Branch: branchFunc(), // No factory dependencies - Executable: func() string { - if exe != "" { - return exe - } - exe = executable("gh") - return exe - }, + Config: configFunc(), // No factory dependencies + Branch: branchFunc(), // No factory dependencies + ExecutableName: "gh", } f.IOStreams = ioStreams(f) // Depends on Config @@ -121,52 +113,6 @@ func browserLauncher(f *cmdutil.Factory) string { return os.Getenv("BROWSER") } -// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. -// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in -// PATH, return the absolute location to the program. -// -// The idea is that the result of this function is callable in the future and refers to the same -// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software -// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. -// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of -// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew -// location. -// -// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute -// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git -// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh -// auth login`, running `brew update` will print out authentication errors as git is unable to locate -// Homebrew-installed `gh`. -func executable(fallbackName string) string { - exe, err := os.Executable() - if err != nil { - return fallbackName - } - - base := filepath.Base(exe) - path := os.Getenv("PATH") - for _, dir := range filepath.SplitList(path) { - p, err := filepath.Abs(filepath.Join(dir, base)) - if err != nil { - continue - } - f, err := os.Stat(p) - if err != nil { - continue - } - - if p == exe { - return p - } else if f.Mode()&os.ModeSymlink != 0 { - if t, err := os.Readlink(p); err == nil && t == exe { - return p - } - } - } - - return exe -} - func configFunc() func() (config.Config, error) { var cachedConfig config.Config var configError error diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 83d4a638f..73cee489a 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -2,6 +2,9 @@ package cmdutil import ( "net/http" + "os" + "path/filepath" + "strings" "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/config" @@ -25,7 +28,59 @@ type Factory struct { Branch func() (string, error) ExtensionManager extensions.ExtensionManager - - // Executable is the path to the currently invoked gh binary - Executable func() string + ExecutableName string +} + +// Executable is the path to the currently invoked binary +func (f *Factory) Executable() string { + if !strings.ContainsRune(f.ExecutableName, os.PathSeparator) { + f.ExecutableName = executable(f.ExecutableName) + } + return f.ExecutableName +} + +// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. +// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in +// PATH, return the absolute location to the program. +// +// The idea is that the result of this function is callable in the future and refers to the same +// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software +// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. +// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of +// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew +// location. +// +// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute +// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git +// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh +// auth login`, running `brew update` will print out authentication errors as git is unable to locate +// Homebrew-installed `gh`. +func executable(fallbackName string) string { + exe, err := os.Executable() + if err != nil { + return fallbackName + } + + base := filepath.Base(exe) + path := os.Getenv("PATH") + for _, dir := range filepath.SplitList(path) { + p, err := filepath.Abs(filepath.Join(dir, base)) + if err != nil { + continue + } + f, err := os.Stat(p) + if err != nil { + continue + } + + if p == exe { + return p + } else if f.Mode()&os.ModeSymlink != 0 { + if t, err := os.Readlink(p); err == nil && t == exe { + return p + } + } + } + + return exe } From 3b7e5fc24678c8e6558ddd42c565f8280d73f8e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Dec 2021 14:03:10 +0100 Subject: [PATCH 052/167] Store Executable() information on codespaces App This is to avoid having to explicitly pass it to each subcommand that needs it. Each codespaces command runs in the context of App, so that's a point of shared context that we can store state in. --- pkg/cmd/codespace/common.go | 22 ++++++++++++++-------- pkg/cmd/codespace/delete_test.go | 2 +- pkg/cmd/codespace/root.go | 5 ++--- pkg/cmd/codespace/ssh.go | 13 +++++++------ pkg/cmd/root/root.go | 3 ++- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 09b9308aa..1107ae6a5 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -21,19 +21,25 @@ import ( "golang.org/x/term" ) -type App struct { - io *iostreams.IOStreams - apiClient apiClient - errLogger *log.Logger +type executable interface { + Executable() string } -func NewApp(io *iostreams.IOStreams, apiClient apiClient) *App { +type App struct { + io *iostreams.IOStreams + apiClient apiClient + errLogger *log.Logger + executable executable +} + +func NewApp(io *iostreams.IOStreams, exe executable, apiClient apiClient) *App { errLogger := log.New(io.ErrOut, "", 0) return &App{ - io: io, - apiClient: apiClient, - errLogger: errLogger, + io: io, + apiClient: apiClient, + errLogger: errLogger, + executable: exe, } } diff --git a/pkg/cmd/codespace/delete_test.go b/pkg/cmd/codespace/delete_test.go index 58090c809..89318b5e3 100644 --- a/pkg/cmd/codespace/delete_test.go +++ b/pkg/cmd/codespace/delete_test.go @@ -190,7 +190,7 @@ func TestDelete(t *testing.T) { io, _, stdout, stderr := iostreams.Test() io.SetStdinTTY(true) io.SetStdoutTTY(true) - app := NewApp(io, apiMock) + app := NewApp(io, nil, apiMock) err := app.Delete(context.Background(), opts) if (err != nil) != tt.wantErr { t.Errorf("delete() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/cmd/codespace/root.go b/pkg/cmd/codespace/root.go index 5a3499dee..5b2c0d8fc 100644 --- a/pkg/cmd/codespace/root.go +++ b/pkg/cmd/codespace/root.go @@ -1,11 +1,10 @@ package codespace import ( - "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) -func NewRootCmd(app *App, f *cmdutil.Factory) *cobra.Command { +func NewRootCmd(app *App) *cobra.Command { root := &cobra.Command{ Use: "codespace", Short: "Connect to and manage your codespaces", @@ -17,7 +16,7 @@ func NewRootCmd(app *App, f *cmdutil.Factory) *cobra.Command { root.AddCommand(newListCmd(app)) root.AddCommand(newLogsCmd(app)) root.AddCommand(newPortsCmd(app)) - root.AddCommand(newSSHCmd(app, f)) + root.AddCommand(newSSHCmd(app)) root.AddCommand(newCpCmd(app)) root.AddCommand(newStopCmd(app)) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 18730a176..19c4930f5 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -34,7 +34,7 @@ type sshOptions struct { scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } -func newSSHCmd(app *App, f *cmdutil.Factory) *cobra.Command { +func newSSHCmd(app *App) *cobra.Command { var opts sshOptions sshCmd := &cobra.Command{ @@ -70,7 +70,7 @@ func newSSHCmd(app *App, f *cmdutil.Factory) *cobra.Command { fmt.Fprintf(os.Stderr, "%v\n", err) } - sshCmd.AddCommand(newConfigCmd(app, f)) + sshCmd.AddCommand(newConfigCmd(app)) return sshCmd } @@ -174,7 +174,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, executable string) error { +func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -258,6 +258,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut return fmt.Errorf("error formatting template: %w", err) } + ghExec := a.executable.Executable() for result := range sshUsers { if result.err != nil { fmt.Fprintf(os.Stderr, "%v\n", result.err) @@ -287,7 +288,7 @@ func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions, execut Name: result.codespace.Name, EscapedRef: strings.ReplaceAll(result.codespace.GitStatus.Ref, "/", "-"), SSHUser: result.user, - GHExec: executable, + GHExec: ghExec, } if err := t.Execute(a.io.Out, conf); err != nil { return err @@ -392,7 +393,7 @@ type configOptions struct { codespace string } -func newConfigCmd(app *App, f *cmdutil.Factory) *cobra.Command { +func newConfigCmd(app *App) *cobra.Command { var opts configOptions configCmd := &cobra.Command{ @@ -418,7 +419,7 @@ func newConfigCmd(app *App, f *cmdutil.Factory) *cobra.Command { $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' `), RunE: func(cmd *cobra.Command, args []string) error { - return app.printOpenSSHConfig(cmd.Context(), opts, f.Executable()) + return app.printOpenSSHConfig(cmd.Context(), opts) }, DisableFlagsInUseLine: true, } diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 58272306c..cff797e93 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -135,6 +135,7 @@ func newCodespaceCmd(f *cmdutil.Factory) *cobra.Command { vscsURL := os.Getenv("INTERNAL_VSCS_TARGET_URL") app := codespaceCmd.NewApp( f.IOStreams, + f, codespacesAPI.New( serverURL, apiURL, @@ -142,7 +143,7 @@ func newCodespaceCmd(f *cmdutil.Factory) *cobra.Command { &lazyLoadedHTTPClient{factory: f}, ), ) - cmd := codespaceCmd.NewRootCmd(app, f) + cmd := codespaceCmd.NewRootCmd(app) cmd.Use = "codespace" cmd.Aliases = []string{"cs"} cmd.Annotations = map[string]string{"IsCore": "true"} From 354bac82b3a513a5796694de262129b40e12ece9 Mon Sep 17 00:00:00 2001 From: Andrei Jiroh Eugenio Halili Date: Fri, 24 Dec 2021 18:43:22 +0800 Subject: [PATCH 053/167] Add Alpine Linux install docs --- docs/install_linux.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/install_linux.md b/docs/install_linux.md index b11c2faa9..eda70db95 100644 --- a/docs/install_linux.md +++ b/docs/install_linux.md @@ -179,6 +179,23 @@ openSUSE Tumbleweed users can install from the [official distribution repo](http sudo zypper in gh ``` +### Alpine Linux + +Alpine Linux users can install from the [stable releases' community packaage repository](https://pkgs.alpinelinux.org/packages?name=github-cli&branch=v3.15). + +```bash +apk add github-cli +``` + +Users wanting the latest version of the CLI without waiting to be backported into their stable release they're using should use the edge release's +community repo through this method below, without mixing packages from stable and unstable repos.[^1] + +```bash +echo "@community http://dl-cdn.alpinelinux.org/alpine/edge/community" >> /etc/apk/repositories +apk add github-cli@community +``` + [releases page]: https://github.com/cli/cli/releases/latest [arch linux repo]: https://www.archlinux.org/packages/community/x86_64/github-cli [arch linux aur]: https://aur.archlinux.org/packages/github-cli-git +[^1]: https://wiki.alpinelinux.org/wiki/Package_management#Repository_pinning From 0ac546a81f266f50025c1288bbc45c1c9a524599 Mon Sep 17 00:00:00 2001 From: Frank Dietrich Date: Thu, 6 Jan 2022 09:16:27 +0100 Subject: [PATCH 054/167] Amend location of GPG key file Following the Debian [documentation](https://wiki.debian.org/SecureApt) the keyring should be stored in `/etc/apt/trusted.gpg.d/. > In more recent Debian GNU/Linux versions (Wheezy, for example), the keyrings are stored in specific files all located in the `/etc/apt/trusted.gpg.d` directory. --- docs/install_linux.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/install_linux.md b/docs/install_linux.md index b11c2faa9..729edf78d 100644 --- a/docs/install_linux.md +++ b/docs/install_linux.md @@ -14,8 +14,8 @@ our release schedule. Install: ```bash -curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg | sudo dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg -echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" | sudo tee /etc/apt/sources.list.d/github-cli.list > /dev/null +curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg | sudo dd of=/etc/apt/trusted.gpg.d/githubcli-archive-keyring.gpg +echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/trusted.gpg.d/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" | sudo tee /etc/apt/sources.list.d/github-cli.list > /dev/null sudo apt update sudo apt install gh ``` From 61e5fbb00764d9ed9b9b8d1c443487eb131edda4 Mon Sep 17 00:00:00 2001 From: Jason Lunz Date: Thu, 6 Jan 2022 10:01:58 -0700 Subject: [PATCH 055/167] Revert "move `gh cs ssh --config` into a separate `gh cs ssh config` command" This reverts commit c9d0085e57a153d30722f4f99d431d7078db9da7. --- pkg/cmd/codespace/ssh.go | 83 +++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 44 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 19c4930f5..726f2152f 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -31,6 +31,7 @@ type sshOptions struct { debug bool debugFile string stdio bool + config bool scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } @@ -40,11 +41,34 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd := &cobra.Command{ Use: "ssh [...] [-- ...] []", Short: "SSH into a codespace", + Long: heredoc.Doc(` + The 'ssh' command is used to SSH into a codespace. In its simplest form, you can + run 'gh cs ssh', select a codespace interactively, and connect. + + The 'ssh' command also supports deeper integration with OpenSSH using a + '--config' option that generates per-codespace ssh configuration in OpenSSH + format. Including this configuration in your ~/.ssh/config improves the user + experience of tools that integrate with OpenSSH, such as bash/zsh completion of + ssh hostnames, remote path completion for scp/rsync/sshfs, git ssh remotes, and + so on. + + Once that is set up (see the second example below), you can ssh to codespaces as + if they were ordinary remote hosts (using 'ssh', not 'gh cs ssh'). + `), + Example: heredoc.Doc(` + $ gh codespace ssh + + $ gh codespace ssh --config > ~/.ssh/codespaces + $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' + `), PreRunE: func(c *cobra.Command, args []string) error { if opts.stdio { if opts.codespace == "" { return errors.New("`--stdio` requires explicit `--codespace`") } + if opts.config { + return errors.New("cannot use `--stdio` with `--config`") + } if opts.serverPort != 0 { return errors.New("cannot use `--stdio` with `--server-port`") } @@ -52,10 +76,22 @@ func newSSHCmd(app *App) *cobra.Command { return errors.New("cannot use `--stdio` with `--profile`") } } + if opts.config { + if opts.profile != "" { + return errors.New("cannot use `--config` with `--profile`") + } + if opts.serverPort != 0 { + return errors.New("cannot use `--config` with `--server-port`") + } + } return nil }, RunE: func(cmd *cobra.Command, args []string) error { - return app.SSH(cmd.Context(), args, opts) + if opts.config { + return app.printOpenSSHConfig(cmd.Context(), opts) + } else { + return app.SSH(cmd.Context(), args, opts) + } }, DisableFlagsInUseLine: true, } @@ -65,13 +101,12 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") + sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") sshCmd.Flags().BoolVar(&opts.stdio, "stdio", false, "Proxy sshd connection to stdio") if err := sshCmd.Flags().MarkHidden("stdio"); err != nil { fmt.Fprintf(os.Stderr, "%v\n", err) } - sshCmd.AddCommand(newConfigCmd(app)) - return sshCmd } @@ -174,7 +209,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } -func (a *App) printOpenSSHConfig(ctx context.Context, opts configOptions) error { +func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) error { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -389,46 +424,6 @@ func (a *App) Copy(ctx context.Context, args []string, opts cpOptions) error { return a.SSH(ctx, nil, opts.sshOptions) } -type configOptions struct { - codespace string -} - -func newConfigCmd(app *App) *cobra.Command { - var opts configOptions - - configCmd := &cobra.Command{ - Use: "config [-c codespace]", - Short: "Write OpenSSH configuration to stdout", - Long: heredoc.Doc(` - The config command generates per-codespace ssh configuration in OpenSSH format. - - Including this configuration in ~/.ssh/config improves the user experience of - tools that integrate with OpenSSH, such as bash/zsh completion of ssh hostnames, - remote path completion for scp/rsync/sshfs, git ssh remotes, and so on. - - If -c/--codespace is specified, configuration is generated for that codespace - only. Otherwise configuration is emitted for all available codespaces. - - When generating configuration for all codespaces, ones that aren't in - "Available" state are skipped because it's necessary to start the codespace to - determine its remote ssh username. However, when using '-c' to generate - configuration for a single codespace, it will be started if necessary. - `), - Example: heredoc.Doc(` - $ gh codespace config > ~/.ssh/codespaces - $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' - `), - RunE: func(cmd *cobra.Command, args []string) error { - return app.printOpenSSHConfig(cmd.Context(), opts) - }, - DisableFlagsInUseLine: true, - } - - configCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of a codespace") - - return configCmd -} - // fileLogger is a wrapper around an log.Logger configured to write // to a file. It exports two additional methods to get the log file name // and close the file handle when the operation is finished. From 015b9f7fea68cbbeb324556f2101d57f73a544db Mon Sep 17 00:00:00 2001 From: keijun-kumagai Date: Tue, 11 Jan 2022 01:49:19 +0900 Subject: [PATCH 056/167] fix(release): discussion category with assets --- pkg/cmd/release/create/create.go | 7 ++++++- pkg/cmd/release/create/http.go | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 1b834373f..bdfdf1460 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -418,7 +418,12 @@ func createRun(opts *CreateOptions) error { } if !opts.Draft { - rel, err := publishRelease(httpClient, newRelease.APIURL) + params := map[string]interface{}{} + params["draft"] = false + if opts.DiscussionCategory != "" { + params["discussion_category_name"] = opts.DiscussionCategory + } + rel, err := publishRelease(httpClient, newRelease.APIURL, params) if err != nil { return err } diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index fb082cd79..058b38c5b 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -134,8 +134,9 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st return &newRelease, err } -func publishRelease(httpClient *http.Client, releaseURL string) (*shared.Release, error) { - req, err := http.NewRequest("PATCH", releaseURL, bytes.NewBufferString(`{"draft":false}`)) +func publishRelease(httpClient *http.Client, releaseURL string, params map[string]interface{}) (*shared.Release, error) { + bodyBytes, err := json.Marshal(params) + req, err := http.NewRequest("PATCH", releaseURL, bytes.NewBuffer(bodyBytes)) if err != nil { return nil, err } From 562f1b3d0d18896d70427d77e60365f74f4b9268 Mon Sep 17 00:00:00 2001 From: nate smith Date: Tue, 11 Jan 2022 14:56:58 -0600 Subject: [PATCH 057/167] add GetOrDefault functionality to config --- internal/config/config_file_test.go | 18 +++++++++--------- internal/config/config_type.go | 3 +++ internal/config/config_type_test.go | 6 +++--- internal/config/from_env.go | 18 ++++++++++++++++++ internal/config/from_env_test.go | 4 ++-- internal/config/from_file.go | 21 +++++++++++++++++---- internal/config/stub.go | 18 ++++++++++++++++++ pkg/cmd/auth/gitcredential/helper.go | 6 +++--- pkg/cmd/auth/gitcredential/helper_test.go | 8 +++++++- pkg/cmd/auth/login/login.go | 2 +- pkg/cmd/auth/logout/logout.go | 2 +- pkg/cmd/auth/refresh/refresh.go | 8 ++++---- pkg/cmd/auth/shared/login_flow.go | 4 ++-- pkg/cmd/auth/shared/login_flow_test.go | 2 +- pkg/cmd/auth/status/status.go | 4 ++-- pkg/cmd/config/get/get.go | 2 +- pkg/cmd/config/get/get_test.go | 2 +- pkg/cmd/config/list/list.go | 2 +- pkg/cmd/config/set/set_test.go | 4 ++-- pkg/cmd/extension/manager.go | 2 +- pkg/cmd/factory/default.go | 6 +++--- pkg/cmd/factory/http.go | 6 +++--- pkg/cmd/factory/http_test.go | 2 +- pkg/cmd/factory/remote_resolver.go | 2 +- pkg/cmd/gist/clone/clone.go | 2 +- pkg/cmd/pr/checkout/checkout.go | 2 +- pkg/cmd/pr/create/create.go | 2 +- pkg/cmd/repo/clone/clone.go | 4 ++-- pkg/cmd/repo/create/create.go | 4 ++-- pkg/cmd/repo/fork/fork.go | 2 +- pkg/cmd/repo/rename/rename.go | 2 +- pkg/cmdutil/auth_check.go | 2 +- pkg/cmdutil/legacy.go | 2 +- 33 files changed, 116 insertions(+), 58 deletions(-) diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 4c35f24f9..8ee938f31 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -22,10 +22,10 @@ hosts: `, "")() config, err := parseConfig("config.yml") assert.NoError(t, err) - user, err := config.Get("github.com", "user") + user, err := config.GetOrDefault("github.com", "user") assert.NoError(t, err) assert.Equal(t, "monalisa", user) - token, err := config.Get("github.com", "oauth_token") + token, err := config.GetOrDefault("github.com", "oauth_token") assert.NoError(t, err) assert.Equal(t, "OTOKEN", token) } @@ -42,10 +42,10 @@ hosts: `, "")() config, err := parseConfig("config.yml") assert.NoError(t, err) - user, err := config.Get("github.com", "user") + user, err := config.GetOrDefault("github.com", "user") assert.NoError(t, err) assert.Equal(t, "monalisa", user) - token, err := config.Get("github.com", "oauth_token") + token, err := config.GetOrDefault("github.com", "oauth_token") assert.NoError(t, err) assert.Equal(t, "OTOKEN", token) } @@ -58,10 +58,10 @@ github.com: `)() config, err := parseConfig("config.yml") assert.NoError(t, err) - user, err := config.Get("github.com", "user") + user, err := config.GetOrDefault("github.com", "user") assert.NoError(t, err) assert.Equal(t, "monalisa", user) - token, err := config.Get("github.com", "oauth_token") + token, err := config.GetOrDefault("github.com", "oauth_token") assert.NoError(t, err) assert.Equal(t, "OTOKEN", token) } @@ -80,13 +80,13 @@ example.com: `)() config, err := parseConfig("config.yml") assert.NoError(t, err) - val, err := config.Get("example.com", "git_protocol") + val, err := config.GetOrDefault("example.com", "git_protocol") assert.NoError(t, err) assert.Equal(t, "https", val) - val, err = config.Get("github.com", "git_protocol") + val, err = config.GetOrDefault("github.com", "git_protocol") assert.NoError(t, err) assert.Equal(t, "ssh", val) - val, err = config.Get("nonexistent.io", "git_protocol") + val, err = config.GetOrDefault("nonexistent.io", "git_protocol") assert.NoError(t, err) assert.Equal(t, "ssh", val) } diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 92792e93f..7a71e0c96 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -9,7 +9,10 @@ import ( // This interface describes interacting with some persistent configuration for gh. type Config interface { Get(string, string) (string, error) + GetOrDefault(string, string) (string, error) GetWithSource(string, string) (string, string, error) + GetOrDefaultWithSource(string, string) (string, string, error) + Default(string) string Set(string, string, string) error UnsetHost(string) Hosts() ([]string, error) diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index bf53aabe4..dd46a7c2e 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -58,11 +58,11 @@ func Test_defaultConfig(t *testing.T) { assert.Equal(t, expected, mainBuf.String()) assert.Equal(t, "", hostsBuf.String()) - proto, err := cfg.Get("", "git_protocol") + proto, err := cfg.GetOrDefault("", "git_protocol") assert.NoError(t, err) assert.Equal(t, "https", proto) - editor, err := cfg.Get("", "editor") + editor, err := cfg.GetOrDefault("", "editor") assert.NoError(t, err) assert.Equal(t, "", editor) @@ -72,7 +72,7 @@ func Test_defaultConfig(t *testing.T) { expansion, _ := aliases.Get("co") assert.Equal(t, expansion, "pr checkout") - browser, err := cfg.Get("", "browser") + browser, err := cfg.GetOrDefault("", "browser") assert.NoError(t, err) assert.Equal(t, "", browser) } diff --git a/internal/config/from_env.go b/internal/config/from_env.go index ad31537f4..27cf3c54b 100644 --- a/internal/config/from_env.go +++ b/internal/config/from_env.go @@ -76,6 +76,24 @@ func (c *envConfig) GetWithSource(hostname, key string) (string, string, error) return c.Config.GetWithSource(hostname, key) } +func (c *envConfig) GetOrDefault(hostname, key string) (val string, err error) { + val, _, err = c.GetOrDefaultWithSource(hostname, key) + return +} + +func (c *envConfig) GetOrDefaultWithSource(hostname, key string) (val string, src string, err error) { + val, src, err = c.GetWithSource(hostname, key) + if err == nil && val == "" { + val = c.Default(key) + } + + return +} + +func (c *envConfig) Default(key string) string { + return c.Config.Default(key) +} + func (c *envConfig) CheckWriteable(hostname, key string) error { if hostname != "" && key == "oauth_token" { if token, env := AuthTokenFromEnv(hostname); token != "" { diff --git a/internal/config/from_env_test.go b/internal/config/from_env_test.go index bf81c7976..59856021d 100644 --- a/internal/config/from_env_test.go +++ b/internal/config/from_env_test.go @@ -301,11 +301,11 @@ func TestInheritEnv(t *testing.T) { hosts, _ := cfg.Hosts() assert.Equal(t, tt.wants.hosts, hosts) - val, source, _ := cfg.GetWithSource(tt.hostname, "oauth_token") + val, source, _ := cfg.GetOrDefaultWithSource(tt.hostname, "oauth_token") assert.Equal(t, tt.wants.token, val) assert.Regexp(t, tt.wants.source, source) - val, _ = cfg.Get(tt.hostname, "oauth_token") + val, _ = cfg.GetOrDefault(tt.hostname, "oauth_token") assert.Equal(t, tt.wants.token, val) err := cfg.CheckWriteable(tt.hostname, "oauth_token") diff --git a/internal/config/from_file.go b/internal/config/from_file.go index 080143df4..3c1cfd65b 100644 --- a/internal/config/from_file.go +++ b/internal/config/from_file.go @@ -65,13 +65,26 @@ func (c *fileConfig) GetWithSource(hostname, key string) (string, string, error) return "", defaultSource, err } - if value == "" { - return defaultFor(key), defaultSource, nil - } - return value, defaultSource, nil } +func (c *fileConfig) GetOrDefault(hostname, key string) (val string, err error) { + val, _, err = c.GetOrDefaultWithSource(hostname, key) + return +} + +func (c *fileConfig) GetOrDefaultWithSource(hostname, key string) (val string, src string, err error) { + val, src, err = c.GetWithSource(hostname, key) + if err != nil && val == "" { + val = c.Default(key) + } + return +} + +func (c *fileConfig) Default(key string) string { + return defaultFor(key) +} + func (c *fileConfig) Set(hostname, key, value string) error { if hostname == "" { return c.SetStringValue(key, value) diff --git a/internal/config/stub.go b/internal/config/stub.go index e68183d32..357458f60 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -25,6 +25,24 @@ func (c ConfigStub) GetWithSource(host, key string) (string, string, error) { return "", "", errors.New("not found") } +func (c ConfigStub) GetOrDefault(hostname, key string) (val string, err error) { + val, _, err = c.GetOrDefaultWithSource(hostname, key) + return +} + +func (c ConfigStub) GetOrDefaultWithSource(hostname, key string) (val string, src string, err error) { + val, src, err = c.GetWithSource(hostname, key) + if err == nil && val == "" { + val = c.Default(key) + } + return +} + +func (c ConfigStub) Default(key string) string { + // TODO may regret this + return defaultFor(key) +} + func (c ConfigStub) Set(host, key, value string) error { c[genKey(host, key)] = value return nil diff --git a/pkg/cmd/auth/gitcredential/helper.go b/pkg/cmd/auth/gitcredential/helper.go index 8d1ab7ff3..67c1c28c4 100644 --- a/pkg/cmd/auth/gitcredential/helper.go +++ b/pkg/cmd/auth/gitcredential/helper.go @@ -14,7 +14,7 @@ import ( const tokenUser = "x-access-token" type config interface { - GetWithSource(string, string) (string, string, error) + GetOrDefaultWithSource(string, string) (string, string, error) } type CredentialOptions struct { @@ -101,11 +101,11 @@ func helperRun(opts *CredentialOptions) error { } var gotUser string - gotToken, source, _ := cfg.GetWithSource(wants["host"], "oauth_token") + gotToken, source, _ := cfg.GetOrDefaultWithSource(wants["host"], "oauth_token") if strings.HasSuffix(source, "_TOKEN") { gotUser = tokenUser } else { - gotUser, _, _ = cfg.GetWithSource(wants["host"], "user") + gotUser, _, _ = cfg.GetOrDefaultWithSource(wants["host"], "user") } if gotUser == "" || gotToken == "" { diff --git a/pkg/cmd/auth/gitcredential/helper_test.go b/pkg/cmd/auth/gitcredential/helper_test.go index 7e30ec495..053dd8f69 100644 --- a/pkg/cmd/auth/gitcredential/helper_test.go +++ b/pkg/cmd/auth/gitcredential/helper_test.go @@ -8,12 +8,18 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" ) +// why not just use the config stub argh type tinyConfig map[string]string -func (c tinyConfig) GetWithSource(host, key string) (string, string, error) { +func (c tinyConfig) GetOrDefaultWithSource(host, key string) (string, string, error) { return c[fmt.Sprintf("%s:%s", host, key)], c["_source"], nil } +func (c tinyConfig) GetOrDefault(host, key string) (val string, err error) { + val, _, err = c.GetOrDefaultWithSource(host, key) + return +} + func Test_helperRun(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index f591fcbc6..f8896de1a 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -165,7 +165,7 @@ func loginRun(opts *LoginOptions) error { return cfg.Write() } - existingToken, _ := cfg.Get(hostname, "oauth_token") + existingToken, _ := cfg.GetOrDefault(hostname, "oauth_token") if existingToken != "" && opts.Interactive { if err := shared.HasMinimumScopes(httpClient, hostname, existingToken); err == nil { var keepGoing bool diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index 3873da324..f48e59db2 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -127,7 +127,7 @@ func logoutRun(opts *LogoutOptions) error { if err != nil { // suppressing; the user is trying to delete this token and it might be bad. // we'll see if the username is in the config and fall back to that. - username, _ = cfg.Get(hostname, "user") + username, _ = cfg.GetOrDefault(hostname, "user") } usernameStr := "" diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index f2b9cbbb4..c4b15003e 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -132,7 +132,7 @@ func refreshRun(opts *RefreshOptions) error { } var additionalScopes []string - if oldToken, _ := cfg.Get(hostname, "oauth_token"); oldToken != "" { + if oldToken, _ := cfg.GetOrDefault(hostname, "oauth_token"); oldToken != "" { if oldScopes, err := shared.GetScopes(opts.httpClient, hostname, oldToken); err == nil { for _, s := range strings.Split(oldScopes, ",") { s = strings.TrimSpace(s) @@ -146,7 +146,7 @@ func refreshRun(opts *RefreshOptions) error { credentialFlow := &shared.GitCredentialFlow{ Executable: opts.MainExecutable, } - gitProtocol, _ := cfg.Get(hostname, "git_protocol") + gitProtocol, _ := cfg.GetOrDefault(hostname, "git_protocol") if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err @@ -159,8 +159,8 @@ func refreshRun(opts *RefreshOptions) error { } if credentialFlow.ShouldSetup() { - username, _ := cfg.Get(hostname, "user") - password, _ := cfg.Get(hostname, "oauth_token") + username, _ := cfg.GetOrDefault(hostname, "user") + password, _ := cfg.GetOrDefault(hostname, "oauth_token") if err := credentialFlow.Setup(hostname, username, password); err != nil { return err } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index 0bac49b35..2ba31b1a9 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -15,7 +15,7 @@ import ( ) type iconfig interface { - Get(string, string) (string, error) + GetOrDefault(string, string) (string, error) Set(string, string, string) error Write() error } @@ -147,7 +147,7 @@ func Login(opts *LoginOptions) error { var username string if userValidated { - username, _ = cfg.Get(hostname, "user") + username, _ = cfg.GetOrDefault(hostname, "user") } else { apiClient := api.NewClientFromHTTP(httpClient) var err error diff --git a/pkg/cmd/auth/shared/login_flow_test.go b/pkg/cmd/auth/shared/login_flow_test.go index 530e34045..1290aa176 100644 --- a/pkg/cmd/auth/shared/login_flow_test.go +++ b/pkg/cmd/auth/shared/login_flow_test.go @@ -17,7 +17,7 @@ import ( type tinyConfig map[string]string -func (c tinyConfig) Get(host, key string) (string, error) { +func (c tinyConfig) GetOrDefault(host, key string) (string, error) { return c[fmt.Sprintf("%s:%s", host, key)], nil } diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index f84004c87..7196d42bc 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -92,7 +92,7 @@ func statusRun(opts *StatusOptions) error { } isHostnameFound = true - token, tokenSource, _ := cfg.GetWithSource(hostname, "oauth_token") + token, tokenSource, _ := cfg.GetOrDefaultWithSource(hostname, "oauth_token") tokenIsWriteable := cfg.CheckWriteable(hostname, "oauth_token") == nil statusInfo[hostname] = []string{} @@ -127,7 +127,7 @@ func statusRun(opts *StatusOptions) error { addMsg("%s %s: api call failed: %s", cs.Red("X"), hostname, err) } addMsg("%s Logged in to %s as %s (%s)", cs.SuccessIcon(), hostname, cs.Bold(username), tokenSource) - proto, _ := cfg.Get(hostname, "git_protocol") + proto, _ := cfg.GetOrDefault(hostname, "git_protocol") if proto != "" { addMsg("%s Git operations for %s configured to use %s protocol.", cs.SuccessIcon(), hostname, cs.Bold(proto)) diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index 3a5634458..94694adb2 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -53,7 +53,7 @@ func NewCmdConfigGet(f *cmdutil.Factory, runF func(*GetOptions) error) *cobra.Co } func getRun(opts *GetOptions) error { - val, err := opts.Config.Get(opts.Hostname, opts.Key) + val, err := opts.Config.GetOrDefault(opts.Hostname, opts.Key) if err != nil { return err } diff --git a/pkg/cmd/config/get/get_test.go b/pkg/cmd/config/get/get_test.go index 7c5efa9be..f376c773d 100644 --- a/pkg/cmd/config/get/get_test.go +++ b/pkg/cmd/config/get/get_test.go @@ -115,7 +115,7 @@ func Test_getRun(t *testing.T) { assert.NoError(t, err) assert.Equal(t, tt.stdout, stdout.String()) assert.Equal(t, tt.stderr, stderr.String()) - _, err = tt.input.Config.Get("", "_written") + _, err = tt.input.Config.GetOrDefault("", "_written") assert.Error(t, err) }) } diff --git a/pkg/cmd/config/list/list.go b/pkg/cmd/config/list/list.go index 8b1157b1f..c2ad0397b 100644 --- a/pkg/cmd/config/list/list.go +++ b/pkg/cmd/config/list/list.go @@ -59,7 +59,7 @@ func listRun(opts *ListOptions) error { configOptions := config.ConfigOptions() for _, key := range configOptions { - val, err := cfg.Get(host, key.Key) + val, err := cfg.GetOrDefault(host, key.Key) if err != nil { return err } diff --git a/pkg/cmd/config/set/set_test.go b/pkg/cmd/config/set/set_test.go index cdd2e7c94..2beb20edc 100644 --- a/pkg/cmd/config/set/set_test.go +++ b/pkg/cmd/config/set/set_test.go @@ -145,11 +145,11 @@ func Test_setRun(t *testing.T) { assert.Equal(t, tt.stdout, stdout.String()) assert.Equal(t, tt.stderr, stderr.String()) - val, err := tt.input.Config.Get(tt.input.Hostname, tt.input.Key) + val, err := tt.input.Config.GetOrDefault(tt.input.Hostname, tt.input.Key) assert.NoError(t, err) assert.Equal(t, tt.expectedValue, val) - val, err = tt.input.Config.Get("", "_written") + val, err = tt.input.Config.GetOrDefault("", "_written") assert.NoError(t, err) assert.Equal(t, "true", val) }) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 691110656..5652fb564 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -337,7 +337,7 @@ func (m *Manager) Install(repo ghrepo.Interface) error { return errors.New("extension is uninstallable: missing executable") } - protocol, _ := m.config.Get(repo.RepoHost(), "git_protocol") + protocol, _ := m.config.GetOrDefault(repo.RepoHost(), "git_protocol") return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), m.io.Out, m.io.ErrOut) } diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 08d93c2be..8c15e0235 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -113,7 +113,7 @@ func browserLauncher(f *cmdutil.Factory) string { cfg, err := f.Config() if err == nil { - if cfgBrowser, _ := cfg.Get("", "browser"); cfgBrowser != "" { + if cfgBrowser, _ := cfg.GetOrDefault("", "browser"); cfgBrowser != "" { return cfgBrowser } } @@ -220,7 +220,7 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { return io } - if prompt, _ := cfg.Get("", "prompt"); prompt == "disabled" { + if prompt, _ := cfg.GetOrDefault("", "prompt"); prompt == "disabled" { io.SetNeverPrompt(true) } @@ -230,7 +230,7 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { // 3. PAGER if ghPager, ghPagerExists := os.LookupEnv("GH_PAGER"); ghPagerExists { io.SetPager(ghPager) - } else if pager, _ := cfg.Get("", "pager"); pager != "" { + } else if pager, _ := cfg.GetOrDefault("", "pager"); pager != "" { io.SetPager(pager) } diff --git a/pkg/cmd/factory/http.go b/pkg/cmd/factory/http.go index fd61f2dc7..2db083557 100644 --- a/pkg/cmd/factory/http.go +++ b/pkg/cmd/factory/http.go @@ -54,7 +54,7 @@ var timezoneNames = map[int]string{ } type configGetter interface { - Get(string, string) (string, error) + GetOrDefault(string, string) (string, error) } // generic authenticated HTTP client for commands @@ -73,7 +73,7 @@ func NewHTTPClient(io *iostreams.IOStreams, cfg configGetter, appVersion string, // which would use that non-default behavior is right here, and it doesn't // seem worth the cognitive overhead everywhere else just to serve this one // use case. - unixSocket, err := cfg.Get("", "http_unix_socket") + unixSocket, err := cfg.GetOrDefault("", "http_unix_socket") if err != nil { return nil, err } @@ -92,7 +92,7 @@ func NewHTTPClient(io *iostreams.IOStreams, cfg configGetter, appVersion string, api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", appVersion)), api.AddHeaderFunc("Authorization", func(req *http.Request) (string, error) { hostname := ghinstance.NormalizeHostname(getHost(req)) - if token, err := cfg.Get(hostname, "oauth_token"); err == nil && token != "" { + if token, err := cfg.GetOrDefault(hostname, "oauth_token"); err == nil && token != "" { return fmt.Sprintf("token %s", token), nil } return "", nil diff --git a/pkg/cmd/factory/http_test.go b/pkg/cmd/factory/http_test.go index 1505d1b65..57613bd66 100644 --- a/pkg/cmd/factory/http_test.go +++ b/pkg/cmd/factory/http_test.go @@ -157,7 +157,7 @@ func TestNewHTTPClient(t *testing.T) { type tinyConfig map[string]string -func (c tinyConfig) Get(host, key string) (string, error) { +func (c tinyConfig) GetOrDefault(host, key string) (string, error) { return c[fmt.Sprintf("%s:%s", host, key)], nil } diff --git a/pkg/cmd/factory/remote_resolver.go b/pkg/cmd/factory/remote_resolver.go index 44cd0242d..a197c41a2 100644 --- a/pkg/cmd/factory/remote_resolver.go +++ b/pkg/cmd/factory/remote_resolver.go @@ -83,7 +83,7 @@ func (rr *remoteResolver) Resolver() func() (context.Remotes, error) { dummyHostname := "example.com" // any non-github.com hostname is fine here if config.IsHostEnv(src) { return nil, fmt.Errorf("none of the git remotes configured for this repository correspond to the %s environment variable. Try adding a matching remote or unsetting the variable.", src) - } else if v, src, _ := cfg.GetWithSource(dummyHostname, "oauth_token"); v != "" && config.IsEnterpriseEnv(src) { + } else if v, src, _ := cfg.GetOrDefaultWithSource(dummyHostname, "oauth_token"); v != "" && config.IsEnterpriseEnv(src) { return nil, errors.New("set the GH_HOST environment variable to specify which GitHub host to use") } return nil, errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`") diff --git a/pkg/cmd/gist/clone/clone.go b/pkg/cmd/gist/clone/clone.go index cbd14b324..5127dba64 100644 --- a/pkg/cmd/gist/clone/clone.go +++ b/pkg/cmd/gist/clone/clone.go @@ -79,7 +79,7 @@ func cloneRun(opts *CloneOptions) error { if err != nil { return err } - protocol, err := cfg.Get(hostname, "git_protocol") + protocol, err := cfg.GetOrDefault(hostname, "git_protocol") if err != nil { return err } diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index e174ccc9b..1063d9f36 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -85,7 +85,7 @@ func checkoutRun(opts *CheckoutOptions) error { if err != nil { return err } - protocol, _ := cfg.Get(baseRepo.RepoHost(), "git_protocol") + protocol, _ := cfg.GetOrDefault(baseRepo.RepoHost(), "git_protocol") remotes, err := opts.Remotes() if err != nil { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index b9e450e71..9151ceabb 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -701,7 +701,7 @@ func handlePush(opts CreateOptions, ctx CreateContext) error { if err != nil { return err } - cloneProtocol, _ := cfg.Get(headRepo.RepoHost(), "git_protocol") + cloneProtocol, _ := cfg.GetOrDefault(headRepo.RepoHost(), "git_protocol") headRepoURL := ghrepo.FormatRemoteURL(headRepo, cloneProtocol) diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index ebfd65ec1..574159ced 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -121,7 +121,7 @@ func cloneRun(opts *CloneOptions) error { return err } - protocol, err = cfg.Get(repo.RepoHost(), "git_protocol") + protocol, err = cfg.GetOrDefault(repo.RepoHost(), "git_protocol") if err != nil { return err } @@ -156,7 +156,7 @@ func cloneRun(opts *CloneOptions) error { // If the repo is a fork, add the parent as an upstream if canonicalRepo.Parent != nil { - protocol, err := cfg.Get(canonicalRepo.Parent.RepoHost(), "git_protocol") + protocol, err := cfg.GetOrDefault(canonicalRepo.Parent.RepoHost(), "git_protocol") if err != nil { return err } diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index b8d8c764e..67ef94829 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -361,7 +361,7 @@ func createFromScratch(opts *CreateOptions) error { } if opts.Clone { - protocol, err := cfg.Get(repo.RepoHost(), "git_protocol") + protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") if err != nil { return err } @@ -498,7 +498,7 @@ func createFromLocal(opts *CreateOptions) error { fmt.Fprintln(stdout, repo.URL) } - protocol, err := cfg.Get(repo.RepoHost(), "git_protocol") + protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") if err != nil { return err } diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 773f46641..98fc79e2f 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -209,7 +209,7 @@ func forkRun(opts *ForkOptions) error { if err != nil { return err } - protocol, err := cfg.Get(repoToFork.RepoHost(), "git_protocol") + protocol, err := cfg.GetOrDefault(repoToFork.RepoHost(), "git_protocol") if err != nil { return err } diff --git a/pkg/cmd/repo/rename/rename.go b/pkg/cmd/repo/rename/rename.go index d8b0e8123..8bbcc4f9a 100644 --- a/pkg/cmd/repo/rename/rename.go +++ b/pkg/cmd/repo/rename/rename.go @@ -144,7 +144,7 @@ func updateRemote(repo ghrepo.Interface, renamed ghrepo.Interface, opts *RenameO return nil, err } - protocol, err := cfg.Get(repo.RepoHost(), "git_protocol") + protocol, err := cfg.GetOrDefault(repo.RepoHost(), "git_protocol") if err != nil { return nil, err } diff --git a/pkg/cmdutil/auth_check.go b/pkg/cmdutil/auth_check.go index 3da9cfcfd..dc340dc51 100644 --- a/pkg/cmdutil/auth_check.go +++ b/pkg/cmdutil/auth_check.go @@ -24,7 +24,7 @@ func CheckAuth(cfg config.Config) bool { } for _, hostname := range hosts { - token, _ := cfg.Get(hostname, "oauth_token") + token, _ := cfg.GetOrDefault(hostname, "oauth_token") if token != "" { return true } diff --git a/pkg/cmdutil/legacy.go b/pkg/cmdutil/legacy.go index 19400f1cc..617d359b4 100644 --- a/pkg/cmdutil/legacy.go +++ b/pkg/cmdutil/legacy.go @@ -16,7 +16,7 @@ func DetermineEditor(cf func() (config.Config, error)) (string, error) { if err != nil { return "", fmt.Errorf("could not read config: %w", err) } - editorCommand, _ = cfg.Get("", "editor") + editorCommand, _ = cfg.GetOrDefault("", "editor") } return editorCommand, nil From 56522f9f14dda0866c579a9a3fe2072a1a0f60a3 Mon Sep 17 00:00:00 2001 From: nate smith Date: Tue, 11 Jan 2022 14:57:10 -0600 Subject: [PATCH 058/167] formatting --- pkg/cmd/auth/status/status.go | 2 +- pkg/cmd/factory/http_test.go | 4 ++-- pkg/cmd/repo/create/create.go | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 7196d42bc..a7578f1e2 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -35,7 +35,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co Args: cobra.ExactArgs(0), Short: "View authentication status", Long: heredoc.Doc(`Verifies and displays information about your authentication state. - + This command will test your authentication state for each GitHub host that gh knows about and report on any issues. `), diff --git a/pkg/cmd/factory/http_test.go b/pkg/cmd/factory/http_test.go index 57613bd66..f5e11bd1e 100644 --- a/pkg/cmd/factory/http_test.go +++ b/pkg/cmd/factory/http_test.go @@ -95,10 +95,10 @@ func TestNewHTTPClient(t *testing.T) { > Accept: application/vnd.github.merge-info-preview+json, application/vnd.github.nebula-preview > Authorization: token ████████████████████ > User-Agent: GitHub CLI v1.2.3 - + < HTTP/1.1 204 No Content < Date: