STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Address PR comments

Browse source
This commit is contained in:
Sam Coe 2023-11-02 11:17:55 +01:00 • committed by William Martin
parent d2ff55737c
commit eb771aecc9
8 changed files with 226 additions and 131 deletions
Download patch file Download diff file Expand all files Collapse all files

4
cmd/gh/main.go
View file

@ -17,6 +17,7 @@ import (
"github.com/cli/cli/v2/api"
"github.com/cli/cli/v2/internal/build"
"github.com/cli/cli/v2/internal/config"
"github.com/cli/cli/v2/internal/config/migration"
"github.com/cli/cli/v2/internal/update"
"github.com/cli/cli/v2/pkg/cmd/factory"
"github.com/cli/cli/v2/pkg/cmd/root"
@ -57,7 +58,8 @@ func mainRun() exitCode {
ctx := context.Background()
if cfg, err := cmdFactory.Config(); err == nil {
if err := cfg.MigrateMultiAccount(); err != nil {
var m migration.MultiAccount
if err := cfg.Migrate(m); err != nil {
fmt.Fprintf(stderr, "failed to migrate configuration: %s\n", err)
return exitError
}

38
internal/config/auth_config_test.go
View file

@ -319,13 +319,13 @@ func TestLoginAddsHostIfNotAlreadyAdded(t *testing.T) {
require.Contains(t, hosts, "github.com")
}
// This test mimics the behaviour of logging in with a token and not providing
// a git protocol.
func TestLoginAddsUserToConfigWithoutGitProtocolOrSecureStorage(t *testing.T) {
// This test mimics the behaviour of logging in with a token, not providing
// a git protocol, and using secure storage.
func TestLoginAddsUserToConfigWithoutGitProtocolAndWithSecureStorage(t *testing.T) {
// Given we are not logged in
authCfg := newTestAuthConfig(t)
// When we log in without git protocol or secure storage
// When we log in without git protocol and with secure storage
keyring.MockInit()
_, err := authCfg.Login("github.com", "test-user", "test-token", "", true)
require.NoError(t, err)
@ -407,7 +407,8 @@ func TestUserWorksRightAfterMigration(t *testing.T) {
// When we migrate
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
// Then we can still get the user correctly
user, err := authCfg.User("github.com")
@ -423,7 +424,8 @@ func TestGitProtocolWorksRightAfterMigration(t *testing.T) {
// When we migrate
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
// Then we can still get the git protocol correctly
gitProtocol, err := authCfg.cfg.Get([]string{hostsKey, "github.com", gitProtocolKey})
@ -439,7 +441,8 @@ func TestHostsWorksRightAfterMigration(t *testing.T) {
// When we migrate
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
// Then we can still get the hosts correctly
hosts := authCfg.Hosts()
@ -454,7 +457,8 @@ func TestDefaultHostWorksRightAfterMigration(t *testing.T) {
// When we migrate
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
// Then the default host is still the enterprise host
defaultHost, source := authCfg.DefaultHost()
@ -470,7 +474,8 @@ func TestTokenWorksRightAfterMigration(t *testing.T) {
// When we migrate
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
// Then we can still get the token correctly
token, source := authCfg.Token("github.com")
@ -490,7 +495,8 @@ func TestLogoutRigthAfterMigrationRemovesHost(t *testing.T) {
// When we migrate and logout
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
require.NoError(t, authCfg.Logout(host, user))
@ -504,7 +510,8 @@ func TestLoginInsecurePostMigrationUsesConfigForToken(t *testing.T) {
// When we migrate and login with insecure storage
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
insecureStorageUsed, err := authCfg.Login("github.com", "test-user", "test-token", "", false)
@ -522,7 +529,8 @@ func TestLoginPostMigrationSetsGitProtocol(t *testing.T) {
authCfg := newTestAuthConfig(t)
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
_, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false)
require.NoError(t, err)
@ -540,7 +548,8 @@ func TestLoginPostMigrationSetsUser(t *testing.T) {
authCfg := newTestAuthConfig(t)
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
_, err := authCfg.Login("github.com", "test-user", "test-token", "ssh", false)
require.NoError(t, err)
@ -561,7 +570,8 @@ func TestLoginSecurePostMigrationRemovesTokenFromConfig(t *testing.T) {
// When we migrate and login again with secure storage
var m migration.MultiAccount
require.NoError(t, Migrate(authCfg.cfg, m))
c := cfg{authCfg.cfg}
require.NoError(t, c.Migrate(m))
keyring.MockInit()
_, err = authCfg.Login("github.com", "test-user", "test-token", "", true)

60
internal/config/config.go
View file

@ -1,10 +1,10 @@
package config
import (
"fmt"
"os"
"path/filepath"
"github.com/cli/cli/v2/internal/config/migration"
"github.com/cli/cli/v2/internal/keyring"
ghAuth "github.com/cli/go-gh/v2/pkg/auth"
ghConfig "github.com/cli/go-gh/v2/pkg/config"
@ -32,6 +32,7 @@ type Config interface {
GetOrDefault(string, string) (string, error)
Set(string, string, string)
Write() error
Migrate(Migration) error
Aliases() *AliasConfig
Authentication() *AuthConfig
@ -41,8 +42,27 @@ type Config interface {
HTTPUnixSocket(string) string
Pager(string) string
Prompt(string) string
Version() string
}
MigrateMultiAccount() error
// Migration is the interace that config migrations must implement.
//
// Migrations will receive a copy of the config, and should modify that copy
// as necessary. After migration has completed, the modified config contents
// will be used.
//
// The calling code is expected to verify that the current version of the config
// matches the PreVersion of the migration before calling Do, and will set the
// config version to the PostVersion after the migration has completed successfully.
//
//go:generate moq -rm -out migration_mock.go . Migration
type Migration interface {
// PreVersion is the required config version for this to be applied
PreVersion() string
// PostVersion is the config version that must be applied after migration
PostVersion() string
// Do is expected to apply any necessary changes to the config in place
Do(*ghConfig.Config) error
}
func NewConfig() (Config, error) {
@ -132,9 +152,36 @@ func (c *cfg) Prompt(hostname string) string {
return val
}
func (c *cfg) MigrateMultiAccount() error {
var m migration.MultiAccount
return Migrate(c.cfg, m)
func (c *cfg) Version() string {
val, _ := c.GetOrDefault("", versionKey)
return val
}
func (c *cfg) Migrate(m Migration) error {
version := c.Version()
// If migration has already occured then do not attempt to migrate again.
if m.PostVersion() == version {
return nil
}
// If migration is incompatible with current version then return an error.
if m.PreVersion() != version {
return fmt.Errorf("failed to migrate as %q pre migration version did not match config version %q", m.PreVersion(), version)
}
if err := m.Do(c.cfg); err != nil {
return fmt.Errorf("failed to migrate config: %s", err)
}
c.Set("", versionKey, m.PostVersion())
// Then write out our migrated config.
if err := c.Write(); err != nil {
return fmt.Errorf("failed to write config after migration: %s", err)
}
return nil
}
func defaultFor(key string) (string, bool) {
@ -250,7 +297,10 @@ func (c *AuthConfig) SetDefaultHost(host, source string) {
func (c *AuthConfig) Login(hostname, username, token, gitProtocol string, secureStorage bool) (bool, error) {
var setErr error
if secureStorage {
// Set the current active oauth token
if setErr = keyring.Set(keyringServiceName(hostname), "", token); setErr == nil {
// And set the oauth token under the user to support later auth switch
// and logout switch without another migration.
setErr = keyring.Set(keyringServiceName(hostname), username, token)
}
if setErr == nil {

110
internal/config/config_mock.go
View file

@ -38,8 +38,8 @@ var _ Config = &ConfigMock{}
// HTTPUnixSocketFunc: func(s string) string {
// panic("mock out the HTTPUnixSocket method")
// },
// MigrateMultiAccountFunc: func() error {
// panic("mock out the MigrateMultiAccount method")
// MigrateFunc: func(migration Migration) error {
// panic("mock out the Migrate method")
// },
// PagerFunc: func(s string) string {
// panic("mock out the Pager method")
@ -50,6 +50,9 @@ var _ Config = &ConfigMock{}
// SetFunc: func(s1 string, s2 string, s3 string) {
// panic("mock out the Set method")
// },
// VersionFunc: func() string {
// panic("mock out the Version method")
// },
// WriteFunc: func() error {
// panic("mock out the Write method")
// },
@ -81,8 +84,8 @@ type ConfigMock struct {
// HTTPUnixSocketFunc mocks the HTTPUnixSocket method.
HTTPUnixSocketFunc func(s string) string
// MigrateMultiAccountFunc mocks the MigrateMultiAccount method.
MigrateMultiAccountFunc func() error
// MigrateFunc mocks the Migrate method.
MigrateFunc func(migration Migration) error
// PagerFunc mocks the Pager method.
PagerFunc func(s string) string
@ -93,6 +96,9 @@ type ConfigMock struct {
// SetFunc mocks the Set method.
SetFunc func(s1 string, s2 string, s3 string)
// VersionFunc mocks the Version method.
VersionFunc func() string
// WriteFunc mocks the Write method.
WriteFunc func() error
@ -131,8 +137,10 @@ type ConfigMock struct {
// S is the s argument value.
S string
}
// MigrateMultiAccount holds details about calls to the MigrateMultiAccount method.
MigrateMultiAccount []struct {
// Migrate holds details about calls to the Migrate method.
Migrate []struct {
// Migration is the migration argument value.
Migration Migration
}
// Pager holds details about calls to the Pager method.
Pager []struct {
@ -153,22 +161,26 @@ type ConfigMock struct {
// S3 is the s3 argument value.
S3 string
}
// Version holds details about calls to the Version method.
Version []struct {
}
// Write holds details about calls to the Write method.
Write []struct {
}
}
lockAliases sync.RWMutex
lockAuthentication sync.RWMutex
lockBrowser sync.RWMutex
lockEditor sync.RWMutex
lockGetOrDefault sync.RWMutex
lockGitProtocol sync.RWMutex
lockHTTPUnixSocket sync.RWMutex
lockMigrateMultiAccount sync.RWMutex
lockPager sync.RWMutex
lockPrompt sync.RWMutex
lockSet sync.RWMutex
lockWrite sync.RWMutex
lockAliases sync.RWMutex
lockAuthentication sync.RWMutex
lockBrowser sync.RWMutex
lockEditor sync.RWMutex
lockGetOrDefault sync.RWMutex
lockGitProtocol sync.RWMutex
lockHTTPUnixSocket sync.RWMutex
lockMigrate sync.RWMutex
lockPager sync.RWMutex
lockPrompt sync.RWMutex
lockSet sync.RWMutex
lockVersion sync.RWMutex
lockWrite sync.RWMutex
}
// Aliases calls AliasesFunc.
@ -389,30 +401,35 @@ func (mock *ConfigMock) HTTPUnixSocketCalls() []struct {
return calls
}
// MigrateMultiAccount calls MigrateMultiAccountFunc.
func (mock *ConfigMock) MigrateMultiAccount() error {
if mock.MigrateMultiAccountFunc == nil {
panic("ConfigMock.MigrateMultiAccountFunc: method is nil but Config.MigrateMultiAccount was just called")
// Migrate calls MigrateFunc.
func (mock *ConfigMock) Migrate(migration Migration) error {
if mock.MigrateFunc == nil {
panic("ConfigMock.MigrateFunc: method is nil but Config.Migrate was just called")
}
callInfo := struct {
}{}
mock.lockMigrateMultiAccount.Lock()
mock.calls.MigrateMultiAccount = append(mock.calls.MigrateMultiAccount, callInfo)
mock.lockMigrateMultiAccount.Unlock()
return mock.MigrateMultiAccountFunc()
Migration Migration
}{
Migration: migration,
}
mock.lockMigrate.Lock()
mock.calls.Migrate = append(mock.calls.Migrate, callInfo)
mock.lockMigrate.Unlock()
return mock.MigrateFunc(migration)
}
// MigrateMultiAccountCalls gets all the calls that were made to MigrateMultiAccount.
// MigrateCalls gets all the calls that were made to Migrate.
// Check the length with:
//
// len(mockedConfig.MigrateMultiAccountCalls())
func (mock *ConfigMock) MigrateMultiAccountCalls() []struct {
// len(mockedConfig.MigrateCalls())
func (mock *ConfigMock) MigrateCalls() []struct {
Migration Migration
} {
var calls []struct {
Migration Migration
}
mock.lockMigrateMultiAccount.RLock()
calls = mock.calls.MigrateMultiAccount
mock.lockMigrateMultiAccount.RUnlock()
mock.lockMigrate.RLock()
calls = mock.calls.Migrate
mock.lockMigrate.RUnlock()
return calls
}
@ -520,6 +537,33 @@ func (mock *ConfigMock) SetCalls() []struct {
return calls
}
// Version calls VersionFunc.
func (mock *ConfigMock) Version() string {
if mock.VersionFunc == nil {
panic("ConfigMock.VersionFunc: method is nil but Config.Version was just called")
}
callInfo := struct {
}{}
mock.lockVersion.Lock()
mock.calls.Version = append(mock.calls.Version, callInfo)
mock.lockVersion.Unlock()
return mock.VersionFunc()
}
// VersionCalls gets all the calls that were made to Version.
// Check the length with:
//
// len(mockedConfig.VersionCalls())
func (mock *ConfigMock) VersionCalls() []struct {
} {
var calls []struct {
}
mock.lockVersion.RLock()
calls = mock.calls.Version
mock.lockVersion.RUnlock()
return calls
}
// Write calls WriteFunc.
func (mock *ConfigMock) Write() error {
if mock.WriteFunc == nil {

56
internal/config/migrate.go
View file

@ -1,56 +0,0 @@
package config
import (
"fmt"
ghConfig "github.com/cli/go-gh/v2/pkg/config"
)
//go:generate moq -rm -out migration_mock.go . Migration
// Migration is the interace that config migrations must implement.
//
// Migrations will receive a copy of the config, and should modify that copy
// as necessary. After migration has completed, the modified config contents
// will be used.
//
// The calling code is expected to verify that the current version of the config
// matches the PreVersion of the migration before calling Do, and will set the
// config version to the PostVersion after the migration has completed successfully.
type Migration interface {
// PreVersion is the required config version for this to be applied
PreVersion() string
// PostVersion is the config version that must be applied after migration
PostVersion() string
// Do is expected to apply any necessary changes to the config in place
Do(*ghConfig.Config) error
}
func Migrate(c *ghConfig.Config, m Migration) error {
// It is expected initially that there is no version key because we don't
// have one to begin with, so an error is expected.
version, _ := c.Get([]string{versionKey})
// If migration has already occured then do not attempt to migrate again.
if m.PostVersion() == version {
return nil
}
// If migration is incompatible with current version then return an error.
if m.PreVersion() != version {
return fmt.Errorf("failed to migrate as %q pre migration version did not match config version %q", m.PreVersion(), version)
}
if err := m.Do(c); err != nil {
return fmt.Errorf("failed to migrate config: %s", err)
}
c.Set([]string{versionKey}, m.PostVersion())
// Then write out our migrated config.
if err := ghConfig.Write(c); err != nil {
return fmt.Errorf("failed to write config after migration: %s", err)
}
return nil
}

75
internal/config/migrate_test.go
View file

@ -17,7 +17,8 @@ func TestMigrationAppliedSuccessfully(t *testing.T) {
// Given we have a migrator that writes some keys to the top level config
// and hosts key
cfg := ghConfig.ReadFromString(testFullConfig())
c := ghConfig.ReadFromString(testFullConfig())
topLevelKey := []string{"toplevelkey"}
newHostKey := []string{hostsKey, "newhost"}
@ -28,11 +29,12 @@ func TestMigrationAppliedSuccessfully(t *testing.T) {
})
// When we run the migration
require.NoError(t, Migrate(cfg, migration))
conf := cfg{c}
require.NoError(t, conf.Migrate(migration))
// Then our original config is updated with the migration applied
requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, cfg, newHostKey, "newhostvalue")
requireKeyWithValue(t, c, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, c, newHostKey, "newhostvalue")
// And our config / hosts changes are persisted to their relevant files
// Note that this is real janky. We have writers that represent the
@ -54,8 +56,8 @@ func TestMigrationAppliedBumpsVersion(t *testing.T) {
// Given we have a migration with a pre version that matches
// the version in the config
cfg := ghConfig.ReadFromString(testFullConfig())
cfg.Set([]string{versionKey}, "expected-pre-version")
c := ghConfig.ReadFromString(testFullConfig())
c.Set([]string{versionKey}, "expected-pre-version")
topLevelKey := []string{"toplevelkey"}
migration := &MigrationMock{
@ -72,11 +74,12 @@ func TestMigrationAppliedBumpsVersion(t *testing.T) {
}
// When we migrate
require.NoError(t, Migrate(cfg, migration))
conf := cfg{c}
require.NoError(t, conf.Migrate(migration))
// Then our original config is updated with the migration applied
requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, cfg, []string{versionKey}, "expected-post-version")
requireKeyWithValue(t, c, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, c, []string{versionKey}, "expected-post-version")
// And our config / hosts changes are persisted to their relevant files
var configBuf bytes.Buffer
@ -87,13 +90,40 @@ func TestMigrationAppliedBumpsVersion(t *testing.T) {
requireKeyWithValue(t, persistedCfg, []string{versionKey}, "expected-post-version")
}
func TestMigrationIsNoopWhenAlreadyApplied(t *testing.T) {
// Given we have a migration with a post version that matches
// the version in the config
c := ghConfig.ReadFromString(testFullConfig())
c.Set([]string{versionKey}, "expected-post-version")
migration := &MigrationMock{
DoFunc: func(config *ghConfig.Config) error {
return errors.New("is not called")
},
PreVersionFunc: func() string {
return "is not called"
},
PostVersionFunc: func() string {
return "expected-post-version"
},
}
// When we run Migrate
conf := cfg{c}
err := conf.Migrate(migration)
// Then there is nothing done and the config is not modified
require.NoError(t, err)
requireKeyWithValue(t, c, []string{versionKey}, "expected-post-version")
}
func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) {
StubWriteConfig(t)
// Given we have a migration with a pre version that does not match
// the version in the config
cfg := ghConfig.ReadFromString(testFullConfig())
cfg.Set([]string{versionKey}, "not-expected-pre-version")
c := ghConfig.ReadFromString(testFullConfig())
c.Set([]string{versionKey}, "not-expected-pre-version")
topLevelKey := []string{"toplevelkey"}
migration := &MigrationMock{
@ -110,12 +140,13 @@ func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) {
}
// When we run Migrate
err := Migrate(cfg, migration)
conf := cfg{c}
err := conf.Migrate(migration)
// Then there is an error the migration is not applied and the version is not modified
require.ErrorContains(t, err, `failed to migrate as "expected-pre-version" pre migration version did not match config version "not-expected-pre-version"`)
requireNoKey(t, cfg, topLevelKey)
requireKeyWithValue(t, cfg, []string{versionKey}, "not-expected-pre-version")
requireNoKey(t, c, topLevelKey)
requireKeyWithValue(t, c, []string{versionKey}, "not-expected-pre-version")
}
func TestMigrationErrorWritesNoFiles(t *testing.T) {
@ -123,13 +154,14 @@ func TestMigrationErrorWritesNoFiles(t *testing.T) {
t.Setenv("GH_CONFIG_DIR", tempDir)
// Given we have a migrator that errors
cfg := ghConfig.ReadFromString(testFullConfig())
c := ghConfig.ReadFromString(testFullConfig())
migration := mockMigration(func(config *ghConfig.Config) error {
return errors.New("failed to migrate in test")
})
// When we run the migration
err := Migrate(cfg, migration)
conf := cfg{c}
err := conf.Migrate(migration)
// Then the error is wrapped and bubbled
require.EqualError(t, err, "failed to migrate config: failed to migrate in test")
@ -166,18 +198,19 @@ func TestMigrationWriteErrors(t *testing.T) {
// Given we error when writing the files (because we chmod the files as trickery)
makeFileUnwriteable(t, filepath.Join(tempDir, tt.unwriteableFile))
cfg := ghConfig.ReadFromString(testFullConfig())
c := ghConfig.ReadFromString(testFullConfig())
topLevelKey := []string{"toplevelkey"}
hostsKey := []string{hostsKey, "newhost"}
migration := mockMigration(func(c *ghConfig.Config) error {
c.Set(topLevelKey, "toplevelvalue")
c.Set(hostsKey, "newhostvalue")
migration := mockMigration(func(someCfg *ghConfig.Config) error {
someCfg.Set(topLevelKey, "toplevelvalue")
someCfg.Set(hostsKey, "newhostvalue")
return nil
})
// When we run the migration
err := Migrate(cfg, migration)
conf := cfg{c}
err := conf.Migrate(migration)
// Then the error is wrapped and bubbled
require.ErrorContains(t, err, tt.wantErrContains)

7
internal/config/migration/multi_account.go
View file

@ -65,6 +65,8 @@ type MultiAccount struct {
}
func (m MultiAccount) PreVersion() string {
// It is expected that there is no version key since this migration
// introduces it.
return ""
}
@ -149,7 +151,10 @@ func getUsername(c *config.Config, hostname, token string, transport http.RoundT
}
}
err = client.Query("CurrentUser", &query, nil)
return query.Viewer.Login, err
if err != nil {
return "", err
}
return query.Viewer.Login, nil
}
func migrateToken(hostname, username, token string, inKeyring bool) error {

7
internal/config/stub.go
View file

@ -26,6 +26,9 @@ func NewFromString(cfgStr string) *ConfigMock {
mock.WriteFunc = func() error {
return cfg.Write()
}
mock.MigrateFunc = func(m Migration) error {
return cfg.Migrate(m)
}
mock.AliasesFunc = func() *AliasConfig {
return &AliasConfig{cfg: c}
}
@ -69,6 +72,10 @@ func NewFromString(cfgStr string) *ConfigMock {
val, _ := cfg.GetOrDefault(hostname, promptKey)
return val
}
mock.VersionFunc = func() string {
val, _ := cfg.GetOrDefault("", versionKey)
return val
}
return mock
}