Skip to content

Commit

Permalink
Introduce config versioning and no longer write backup
Browse files Browse the repository at this point in the history
  • Loading branch information
@williammartin
williammartin committed Oct 23, 2023
1 parent 45cb1ac commit 59787eb
Show file tree
Hide file tree
Showing 5 changed files with 215 additions and 154 deletions.
69 changes: 18 additions & 51 deletions pkg/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,6 @@ func ReadFromString(str string) *Config {
// Write gh configuration files to the local file system.
// It will only write gh configuration files that have been modified
// since last being read.
//
// Note that a large portion of this is duplicated into the writeBackup
// function so any changes here might also be required there.
func Write(c *Config) error {
c.mu.Lock()
defer c.mu.Unlock()
Expand Down Expand Up @@ -335,11 +332,16 @@ browser:
// as necessary. After migration has completed, the modified config contents
// will be used.
//
// If a migration is not required, because it has already been done, then
// the migration should return false as the first return value. In this case,
// any changes to the copied config will be ignored.
// 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 {
Do(*Config) (bool, error)
// 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(*Config) error
}

func Migrate(c *Config, m Migration) error {
Expand All @@ -350,27 +352,23 @@ func Migrate(c *Config, m Migration) error {
// case of errors.
copiedCfg := c.deepCopyEntries()

requiredMigration, err := m.Do(copiedCfg)
if err != nil {
return fmt.Errorf("failed to migrate config: %s", err)
// Find out version from copiedCfg to avoid deadlock with c.
// 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, _ := copiedCfg.Get([]string{"version"})
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 there was no migration required, we're done
if !requiredMigration {
return nil
if err := m.Do(copiedCfg); err != nil {
return fmt.Errorf("failed to migrate config: %s", err)
}

// Otherwise we'll write our current hosts config to a backup file
if err := writeBackup(c); err != nil {
return fmt.Errorf("failed to write config backup after migration: %s", err)
}
copiedCfg.Set([]string{"version"}, m.PostVersion())

// Then write out our migrated config. Note that since this is a deep copy, all the modified
// flags will be set to true, so we'll write out both config and hosts unconditionally.
if err := Write(copiedCfg); err != nil {
// Note that this error case is not tested as it was not easily possible to force an error
// here, and not on the backup write above, without introducing new abstractions which
// seemed to complicate the whole affair.
return fmt.Errorf("failed to write config after migration: %s", err)
}

Expand All @@ -379,34 +377,3 @@ func Migrate(c *Config, m Migration) error {
c.entries = copiedCfg.entries
return nil
}

// Note that writeBackup is mostly copied from the Write function, so any changes
// here might also be required there.
//
// It differs from the Write function in that it does not consider whether any
// modification of entries has occured, and just writes the files out unconditionally.
func writeBackup(c *Config) error {
hosts, hostsErr := c.entries.FindEntry("hosts")
hadHostsEntry := hostsErr == nil
if hadHostsEntry {
backupFilePath := hostsConfigFile() + ".bak"
if err := writeFile(backupFilePath, []byte(hosts.String())); err != nil {
return fmt.Errorf("failed to write hosts backup: %w", err)
}
}

// Hosts gets written to a different file above so remove it
// before writing and add it back in after writing.
_ = c.entries.RemoveEntry("hosts")

generalConfigFilePath := generalConfigFile() + ".bak"
if err := writeFile(generalConfigFilePath, []byte(c.entries.String())); err != nil {
return fmt.Errorf("failed to write config backup: %w", err)
}

if hadHostsEntry {
c.entries.AddEntry("hosts", hosts)
}

return nil
}
135 changes: 88 additions & 47 deletions pkg/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -646,16 +646,14 @@ func TestMigrationAppliedSuccessfully(t *testing.T) {
topLevelKey := []string{"toplevelkey"}
newHostKey := []string{"hosts", "newhost"}

migrator := &MigrationMock{
DoFunc: func(config *Config) (bool, error) {
config.Set(topLevelKey, "toplevelvalue")
config.Set(newHostKey, "newhostvalue")
return true, nil
},
}
migration := mockMigration(func(config *Config) error {
config.Set(topLevelKey, "toplevelvalue")
config.Set(newHostKey, "newhostvalue")
return nil
})

// When we run the migration
require.NoError(t, Migrate(cfg, migrator))
require.NoError(t, Migrate(cfg, migration))

// Then our original config is updated with the migration applied
requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue")
Expand All @@ -666,35 +664,79 @@ func TestMigrationAppliedSuccessfully(t *testing.T) {
require.NoError(t, err)
requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, persistedCfg, newHostKey, "newhostvalue")
}

func TestMigrationAppliedBumpsVersion(t *testing.T) {
// Create a location for our tests to write,
// Note that using env var here makes these tests unparallelisable.
tempDir := t.TempDir()
t.Setenv("GH_CONFIG_DIR", tempDir)

// Given we have a migration with a pre version that matches
// the version in the config
cfg := ReadFromString(testFullConfig())
cfg.Set([]string{"version"}, "expected-pre-version")
topLevelKey := []string{"toplevelkey"}

// And we have a backup file with the contents of the original config
backupCfg, err := load(filepath.Join(tempDir, "config.yml.bak"), filepath.Join(tempDir, "hosts.yml.bak"))
migration := &MigrationMock{
DoFunc: func(config *Config) error {
config.Set(topLevelKey, "toplevelvalue")
return nil
},
PreVersionFunc: func() string {
return "expected-pre-version"
},
PostVersionFunc: func() string {
return "expected-post-version"
},
}

// When we migrate
require.NoError(t, Migrate(cfg, migration))

// Then our original config is updated with the migration applied
requireKeyWithValue(t, cfg, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, cfg, []string{"version"}, "expected-post-version")

// And our config / hosts changes are persisted to their relevant files
persistedCfg, err := load(generalConfigFile(), hostsConfigFile())
require.NoError(t, err)
requireNoKey(t, backupCfg, topLevelKey)
requireNoKey(t, backupCfg, newHostKey)
requireKeyWithValue(t, persistedCfg, topLevelKey, "toplevelvalue")
requireKeyWithValue(t, persistedCfg, []string{"version"}, "expected-post-version")
}

func TestMigrationNotRequiredWritesNoFiles(t *testing.T) {
func TestMigrationErrorsWhenPreVersionMismatch(t *testing.T) {
// Create a location for our tests to write,
// Note that using env var here makes these tests unparallelisable.
tempDir := t.TempDir()
t.Setenv("GH_CONFIG_DIR", tempDir)

// Given we have a migrator that returns that no migration is required
// Given we have a migration with a pre version that does not match
// the version in the config
cfg := ReadFromString(testFullConfig())
migrator := &MigrationMock{
DoFunc: func(config *Config) (bool, error) {
return false, nil
cfg.Set([]string{"version"}, "not-expected-pre-version")
topLevelKey := []string{"toplevelkey"}

migration := &MigrationMock{
DoFunc: func(config *Config) error {
config.Set(topLevelKey, "toplevelvalue")
return nil
},
PreVersionFunc: func() string {
return "expected-pre-version"
},
PostVersionFunc: func() string {
return "not-expected"
},
}

// When we run the migration
require.NoError(t, Migrate(cfg, migrator))
// When we run Migrate
err := Migrate(cfg, migration)

// Then no files are written to disk
files, err := os.ReadDir(tempDir)
require.NoError(t, err)
require.Len(t, files, 0)
// 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{"version"}, "not-expected-pre-version")
}

func TestMigrationErrorWritesNoFiles(t *testing.T) {
Expand All @@ -705,14 +747,12 @@ func TestMigrationErrorWritesNoFiles(t *testing.T) {

// Given we have a migrator that errors
cfg := ReadFromString(testFullConfig())
migrator := &MigrationMock{
DoFunc: func(config *Config) (bool, error) {
return true, errors.New("failed to migrate in test")
},
}
migration := mockMigration(func(config *Config) error {
return errors.New("failed to migrate in test")
})

// When we run the migration
err := Migrate(cfg, migrator)
err := Migrate(cfg, migration)

// Then the error is wrapped and bubbled
require.EqualError(t, err, "failed to migrate config: failed to migrate in test")
Expand All @@ -729,16 +769,6 @@ func TestMigrationWriteErrors(t *testing.T) {
unwriteableFile string
wantErrContains string
}{
{
name: "failure to write hosts backup",
unwriteableFile: "hosts.yml.bak",
wantErrContains: "failed to write hosts backup",
},
{
name: "failure to write config backup",
unwriteableFile: "config.yml.bak",
wantErrContains: "failed to write config backup",
},
{
name: "failure to write hosts",
unwriteableFile: "hosts.yml",
Expand All @@ -758,20 +788,18 @@ func TestMigrationWriteErrors(t *testing.T) {
tempDir := t.TempDir()
t.Setenv("GH_CONFIG_DIR", tempDir)

// Given we error when writing the hosts backup (because we chmod the files as trickery)
// Given we error when writing the files (because we chmod the files as trickery)
makeFileUnwriteable(t, filepath.Join(tempDir, tt.unwriteableFile))

cfg := ReadFromString(testFullConfig())
topLevelKey := []string{"toplevelkey"}
hostsKey := []string{"hosts", "newhost"}

migration := &MigrationMock{
DoFunc: func(c *Config) (bool, error) {
c.Set(topLevelKey, "toplevelvalue")
c.Set(hostsKey, "newhostvalue")
return true, nil
},
}
migration := mockMigration(func(c *Config) error {
c.Set(topLevelKey, "toplevelvalue")
c.Set(hostsKey, "newhostvalue")
return nil
})

// When we run the migration
err := Migrate(cfg, migration)
Expand Down Expand Up @@ -812,6 +840,19 @@ func makeFileUnwriteable(t *testing.T, file string) {
require.NoError(t, os.Chmod(file, 0000))
}

func mockMigration(doFunc func(config *Config) error) *MigrationMock {
return &MigrationMock{
DoFunc: doFunc,
PreVersionFunc: func() string {
return ""
},
PostVersionFunc: func() string {
return "not-expected"
},
}

}

func testConfig() *Config {
var data = `
git_protocol: ssh
Expand Down
40 changes: 10 additions & 30 deletions pkg/config/migration/multi_account.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,19 @@ var hostsKey = []string{"hosts"}

type MultiAccount struct{}

func (m MultiAccount) PreVersion() string {
return ""
}

func (m MultiAccount) PostVersion() string {
return "1"
}

func (m MultiAccount) Do(c *config.Config) (bool, error) {
hostnames, err := c.Keys(hostsKey)
// [github.com, github.localhost]
// We wouldn't expect to have a hosts key when this is the first time anyone
// is logging in with the CLI.
var keyNotFoundError *config.KeyNotFoundError
if errors.As(err, &keyNotFoundError) {
return false, nil
Expand All @@ -70,15 +80,6 @@ func (m MultiAccount) Do(c *config.Config) (bool, error) {
return false, nil
}

// If there is an active user set for the first host then we must have already
// migrated, so no migration is required. Note that this is a little janky but
// without introducing a version into the hosts (which would require special handling elsewhere),
// or a version into the config (which would require config backup to move in sync with hosts backup)
// it seems like the best option for now.
if _, err := c.Get(append(hostsKey, hostnames[0], "active_user")); err == nil {
return false, nil
}

// Otherwise let's get to the business of migrating!
for _, hostname := range hostnames {
configEntryKeys, err := c.Keys(append(hostsKey, hostname))
Expand Down Expand Up @@ -111,30 +112,9 @@ func (m MultiAccount) Do(c *config.Config) (bool, error) {
return false, CowardlyRefusalError{fmt.Sprintf("couldn't get configuration entry value despite %q / %q existing", hostname, configEntryKey)}
}

// Remove all these entries, because we are going to move
if err := c.Remove(append(hostsKey, hostname, configEntryKey)); err != nil {
return false, CowardlyRefusalError{fmt.Sprintf("couldn't remove configuration entry %q despite %q / %q existing", configEntryKey, hostname, configEntryKey)}
}

// If this is the user key, we don't need to do anything with it because it's
// now part of the final structure.
if configEntryKey == "user" {
continue
}

// And if it's the oauth_token, we want to duplicate it up a layer to ensure the go-gh auth
// package continues to work.
if configEntryKey == "oauth_token" {
c.Set(append(hostsKey, hostname, "oauth_token"), configEntryValue)
}

// Set these entries in their new location under the user
c.Set(append(hostsKey, hostname, "users", username, configEntryKey), configEntryValue)
}

// And after migrating all the existing values, we'll add one new "active" key to indicate the user
// that is active for this host after the migration.
c.Set(append(hostsKey, hostname, "active_user"), username)
}

return true, nil
Expand Down
Loading

0 comments on commit 59787eb

Please sign in to comment.