Support migrations of the config #141

williammartin · 2023-10-18T10:17:43Z

Description

This PR relates to adding multi-account support to the CLI. I am opening it for discussion, not because I am confident is done and ready to merge, though I feel relatively good about the approach.

There are two parts to this PR one:

Adding behaviour into the config package to support migrations
Adding a migration that actually does the config migration

I feel like this is a reasonable split of responsibilities, rather than pushing it all into go-gh/config (which actually tries to be structure agnostic) or pulling it all up to cli/cli (which doesn't know anything about where it would store a backup for example).

I've left comments close to the places that I think are interesting, rather than trying to describe it all here.

williammartin · 2023-10-18T10:18:04Z

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d
 	github.com/muesli/reflow v0.3.0
 	github.com/muesli/termenv v0.13.0
-	github.com/stretchr/testify v1.7.0
+	github.com/stretchr/testify v1.8.4


Required to use require.EqualError

williammartin · 2023-10-18T10:19:58Z

pkg/config/config.go

 // Write gh configuration files to the local file system.
 // It will only write gh configuration files that have been modified
 // since last being read.
-func Write(c *Config) error {
+func Write(c *Config, optFns ...WriteOpt) error {


I'm not a fan of using this pattern here but it seemed like the quickest way to achieve the following:

When I want to write a backup of a config, I want it to be persisted regardless of whether anything was modified or not.

Unfortunately, it also means that we can't only back up the bits that have changes (config.yml vs hosts.yml) easily. See more on a later comment about other approaches I explored.

Feels like an simpler option would be to create a new function called writeBackup with almost duplicate logic as Write except we don't check if the config is modified and we write to .bak file.

I think that's fine. In various attempts I just duplicated the whole logic in line and left a comment saying "this was duplicated over there so if you change something here make sure to look over there too".

williammartin · 2023-10-18T10:26:54Z

pkg/config/config.go

+	Do(*Config) (bool, error)
+}
+
+func Migrate(c *Config, m Migration) error {


So this is where config tries to actually support a migration. I split the migration support from the actual migration itself to try and tease apart the responsibilities. This function provides behaviour desired around a migration (avoiding broken config by taking a copy, not writing files when not required, and writing backup files) but leaves the actual migration to the injected Migration.

I made a couple of decisions here which I could go either way on, but felt reasonable:

We take a copy of the entries to avoid destructive mutation on this config. In practice, any error bubbling out is likely to result in a complete CLI failure but I wanted to try and avoid any chance of us continuing with a broken config resulting in surprise behaviour (and definitely avoiding persisting it)

I left it up to the migrations themselves to indicate whether a migration was required at all. An alternative would be to do a comparison of the entries, or to do an IsModified check but I kind of liked the explicitness.

Both these decisions make sense to me.

williammartin · 2023-10-18T10:29:51Z

pkg/config/config.go

+	Do(*Config) (bool, error)
+}
+
+func Migrate(c *Config, m Migration) error {


I played around with another approach that looked something like this:

type Migration interface { Do(*Config) (bool, error) } type BackupConfigPersister interface { Persist(*Config) error } type ConfigPersister interface { Persist(*Config) error } type Migrator struct { m Migration bcp BackupConfigPersister cp ConfigPersister } func (m Migrator) Migrate(c *Config) error { ... }

This would allow for custom behaviour between backup and config persisting and an obvious test seam. It's not bad but in practice it felt like it was a lot of abstraction for limited gain.

Agreed that this seems a bit overly complex.

williammartin · 2023-10-18T10:30:55Z

pkg/config/config.go

+	}
+
+	// Otherwise we'll write our current hosts config to a backup file
+	if err := writeAssumingLockHeld(c, asBackup()); err != nil {


I made an attempt at writing some logic for "if only hosts.yml has changed then just back that up" but it seemed to add more complexity than it was worth to just avoid backing up the config.yml when it had no changes. 🤷

This feels perfectly reasonable 👍

williammartin · 2023-10-18T10:36:24Z

pkg/config/migration/multi_account.go

+
+type MultiAccount struct{}
+
+func (m MultiAccount) Do(c *config.Config) (bool, error) {


Originally this was implemented in the config package itself, leaning on the private yamlmap entries. I decided to rewrite it to be agnostic of that, because it meant that either our migration was very explicitly private to go-gh or we would expose the inner yamlmap like func Migrate(entries *yamlmap.Map) which felt gross.

In practice, I actually think this version reads much nicer and simpler than the yamlmap version.

williammartin · 2023-10-18T10:37:10Z

pkg/config/migration/multi_account.go

+			// e.g. [williammartin, https, vim] but it's possible that a manually
+			// edited config file might nest further but we don't support that.
+			//
+			// We could consider throwing away the nested values, but I suppose


Want to draw attention to this decision. We could also support it but it would certainly complicate the migration.

Perfectly reasonable to blow up here I think.

williammartin · 2023-10-18T11:06:14Z

pkg/config/migration/multi_account.go

+//
+// The reason for this is that we can then add new users under a host.
+
+type MultiAccount struct{}


So, I put this down in the config package so it could live with this PR but I'm actually in two minds about where it belongs. The config exposed by go-gh is agnostic about its contents except for the hosts key. In some ways it would be nice to abstract that knowledge and use some other generalised state to indicate when a file should be split but that's a topic for another time and too much work anyway.

Beyond that, the contents of the configuration are an application level concern. That's when we provide meaning to the contents via the AuthConfig, or otherwise have expectations that we can read and write specific entries. The result of this is that perhaps this migration actually belongs in cli/cli. The seam is already there to support this.

I love the idea of putting this in cli/cli rather than in cli/go-gh.

williammartin · 2023-10-18T11:07:07Z

pkg/config/migration/multi_account.go

+		return false, nil
+	}
+
+	// If there is an active user set for the first host then we must have already


Want to draw attention to this decision.

Makes sense to me. I am curious if we add in a version what places in gh would actually need to change? I can only think of maybe 1 or 2 off the top of my head.

Yeh so the question is "where does that version live?". Let's say it goes in hosts.yml, then it looks like this, which is fine:

version: v2 github.com: active_user: ... ghe.io: active_user: ...

But the internal config structure is actually this:

... hosts: version: 2 github.com: active_user: ... ghe.io: active_user: ...

Now we have to special case anywhere that uses c.Keys(..., "hosts").

On the other hand, if we put it in the config like:

... hosts_version: 2 hosts: github.com: active_user: ... ghe.io: active_user: ...

Then we have a potential split brain between config.yml and hosts.yml.

In practice I think either work but they both leave me with a bit of 🤔 I'd really be in favour of having a version if we can. It makes future migrations a lot simpler.

I would think putting it in the top level of config.yml is the right spot. Any change to the structure of config.yml or hosts.yml would trigger us bumping that version. We should treat them as a single config structure despite being in two separate files.

Ooh, that's super obvious. I like it, and it makes me happy that I decided the Migration should work with the entire config rather than hardcoding it for just hosts.

williammartin · 2023-10-18T11:08:07Z

pkg/config/migration/multi_account.go

+		configEntryKeys, err := c.Keys(append(hostsKey, hostname))
+		// e.g. [user, git_protocol, editor]
+		if err != nil {
+			return false, CowardlyRefusalError{fmt.Sprintf("couldn't get host configuration despite %q existing", hostname)}


None of these errors are tested. I also made the decision to return false even though a migration was required because it's idiomatic that under error, the other return values are ignored anyway. Didn't want to provide meaning to that value in the case of error. 🤷

williammartin · 2023-10-18T11:09:57Z

pkg/config/config_test.go

+	t.Setenv("GH_CONFIG_DIR", tempDir)
+
+	// Given we error when writing files (because we explicitly disallow it via this chmod)
+	require.NoError(t, os.Chmod(tempDir, 0000))


This doesn't work on Windows apparently. Having an interface where we could inject faults on persistence operations would be handy but I think maybe it's just a lot of extra abstraction for limited gain right now.

We could do something like this to make it testable...

func Migrate(c *Config, m Migration) error { return internalMigrate(c, m, writeFunc, writeBackupFunc) } // write tests with writing errors against internalMigrate instead of Migrate func internalMigrate(c *Config, m Migration, w func(c *Config) error, wb func(c *Config) error) error { ... migration logic ... }

Let me know if that is not clear. I don't have strong opinions on this either, just a lighter weight option than introducing an interface.

It is clear. I'm generally not a fan of testing the internals like this but since our tests already happen to live in the same package (rather than _test) maybe this is a reasonable approach. I'll noodle on it thanks.

Doing this approach didn't allow me control over the specific error paths (for hosts vs config) so I went with an integrated solution of making the files unwriteable.

samcoe

Great start on this work! I left inline comments with my thoughts.

samcoe · 2023-10-18T12:21:40Z

pkg/config/config.go

 // Write gh configuration files to the local file system.
 // It will only write gh configuration files that have been modified
 // since last being read.
-func Write(c *Config) error {
+func Write(c *Config, optFns ...WriteOpt) error {


Feels like an simpler option would be to create a new function called writeBackup with almost duplicate logic as Write except we don't check if the config is modified and we write to .bak file.

samcoe · 2023-10-18T12:39:23Z

pkg/config/config.go

+	Do(*Config) (bool, error)
+}
+
+func Migrate(c *Config, m Migration) error {


Both these decisions make sense to me.

samcoe · 2023-10-18T12:41:08Z

pkg/config/config.go

+
+//go:generate moq -rm -out migration_mock.go . Migration
+type Migration interface {
+	Do(*Config) (bool, error)


This function signature feels a bit odd to me, in that the bool return value is not indicating weather the migration was successful or not, it is meant to indicate if the migration was necessary at all. I wonder if splitting this into separate functions IsNecessary and Do would make this more clear. Obviously we can choose function names as we see fit.

I think that's reasonable. It removes potential confusion about what the X means in return X, err as well.

So the only downside of splitting this is the possibility for someone to call Migrate without checking first. Would adding godoc here be sufficient?

I think documentation here is sufficient 👍

samcoe · 2023-10-18T12:41:40Z

pkg/config/config.go

+	Do(*Config) (bool, error)
+}
+
+func Migrate(c *Config, m Migration) error {


Agreed that this seems a bit overly complex.

samcoe · 2023-10-18T12:49:38Z

pkg/config/config.go

+	}
+
+	// Otherwise we'll write our current hosts config to a backup file
+	if err := writeAssumingLockHeld(c, asBackup()); err != nil {


This feels perfectly reasonable 👍

samcoe · 2023-10-18T13:06:19Z

pkg/config/migration/multi_account.go

+//
+// The reason for this is that we can then add new users under a host.
+
+type MultiAccount struct{}


I love the idea of putting this in cli/cli rather than in cli/go-gh.

samcoe · 2023-10-18T13:06:48Z

pkg/config/migration/multi_account.go

+
+type MultiAccount struct{}
+
+func (m MultiAccount) Do(c *config.Config) (bool, error) {


samcoe · 2023-10-18T13:14:27Z

pkg/config/migration/multi_account.go

+		return false, nil
+	}
+
+	// If there is an active user set for the first host then we must have already


Makes sense to me. I am curious if we add in a version what places in gh would actually need to change? I can only think of maybe 1 or 2 off the top of my head.

samcoe · 2023-10-18T13:21:26Z

pkg/config/migration/multi_account.go

+			// e.g. [williammartin, https, vim] but it's possible that a manually
+			// edited config file might nest further but we don't support that.
+			//
+			// We could consider throwing away the nested values, but I suppose


Perfectly reasonable to blow up here I think.

samcoe · 2023-10-18T13:23:00Z

pkg/config/migration/multi_account.go

+// github.com:
+//   active_user: williammartin
+//	 users:
+//	   williammartin:
+//	     git_protocol: https
+//	     editor: vim
+//
+// github.localhost:
+//	 active_user: monalisa
+//     users:
+//	     monalisa:
+//		   git_protocol: https


Indentation here looks a bit off.

gofmt seems to be really unhappy with my attempts to put nested yaml in comments. I'll give it another go though.

If gofmt is complaining I would say just leave it then... it is just a comment.

williammartin force-pushed the wm/multi-account-migration branch from c665b78 to c3a545a Compare October 18, 2023 10:36

williammartin commented Oct 18, 2023

View reviewed changes

williammartin requested a review from samcoe October 18, 2023 11:11

samcoe reviewed Oct 18, 2023

View reviewed changes

williammartin added 8 commits October 20, 2023 12:10

Add support for config migrations

5613aae

Add multi-account migration

f800d4b

Decouple config Write and writeBackup

81ef06d

Remove no longer valid comment about ReadFromString

43190db

Add tests for write failure paths

3737688

Fix comment formatting on migration explanation

d8bd281

Ensure multi account migration hoists oauth token

5579059

Add godoc for Migration interface

b19be58

williammartin force-pushed the wm/multi-account-migration branch from 2ded945 to b19be58 Compare October 20, 2023 10:11

Do not error migration when no hosts key

45cb1ac

williammartin force-pushed the wm/multi-account-migration branch 2 times, most recently from 59787eb to 908e888 Compare October 23, 2023 10:50

Introduce config versioning and no longer write backup

a698840

williammartin force-pushed the wm/multi-account-migration branch from 908e888 to a698840 Compare October 23, 2023 10:58

williammartin closed this Jan 22, 2024


		type MultiAccount struct{}

		func (m MultiAccount) Do(c *config.Config) (bool, error) {

Support migrations of the config #141

Support migrations of the config #141

Conversation

williammartin commented Oct 18, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin commented Oct 18, 2023 •

edited

Loading