From bf9bd9933eac7acfd7a3ee1f2e8089193511b4cb Mon Sep 17 00:00:00 2001 From: gammazero Date: Mon, 24 May 2021 17:33:37 -0700 Subject: [PATCH 1/3] Expose additional migration APIs Expose migration APIs for reading migration config and creating migration fetchers. This allows implementation of commands and external applications that want to retrieve migrations according to the Migration portion of the IPFS config. This change also moves some functionality that is specific to fetching migrations via IPFS into the `ipfsfetcher` package. --- cmd/ipfs/{migration.go => add_migrations.go} | 139 +------- cmd/ipfs/daemon.go | 15 +- cmd/ipfs/migration_test.go | 312 ------------------ .../migrations/ipfsfetcher/ipfsfetcher.go | 71 +++- .../ipfsfetcher/ipfsfetcher_test.go | 150 ++++++++- repo/fsrepo/migrations/migrations.go | 88 +++++ repo/fsrepo/migrations/migrations_test.go | 212 ++++++++++++ 7 files changed, 522 insertions(+), 465 deletions(-) rename cmd/ipfs/{migration.go => add_migrations.go} (52%) delete mode 100644 cmd/ipfs/migration_test.go diff --git a/cmd/ipfs/migration.go b/cmd/ipfs/add_migrations.go similarity index 52% rename from cmd/ipfs/migration.go rename to cmd/ipfs/add_migrations.go index d4e3e15b9d0..b5b6c31a137 100644 --- a/cmd/ipfs/migration.go +++ b/cmd/ipfs/add_migrations.go @@ -2,17 +2,13 @@ package main import ( "context" - "encoding/json" "errors" "fmt" "io" "io/ioutil" - "net/url" "os" "path/filepath" - "strings" - config "github.com/ipfs/go-ipfs-config" "github.com/ipfs/go-ipfs-files" "github.com/ipfs/go-ipfs/core" "github.com/ipfs/go-ipfs/core/coreapi" @@ -24,140 +20,7 @@ import ( "github.com/libp2p/go-libp2p-core/peer" ) -// readMigrationConfig reads the migration config out of the config, avoiding -// reading anything other than the migration section. That way, we're free to -// make arbitrary changes to all _other_ sections in migrations. -func readMigrationConfig(repoRoot string) (*config.Migration, error) { - var cfg struct { - Migration config.Migration - } - - cfgPath, err := config.Filename(repoRoot) - if err != nil { - return nil, err - } - - cfgFile, err := os.Open(cfgPath) - if err != nil { - return nil, err - } - defer cfgFile.Close() - - err = json.NewDecoder(cfgFile).Decode(&cfg) - if err != nil { - return nil, err - } - - switch cfg.Migration.Keep { - case "": - cfg.Migration.Keep = config.DefaultMigrationKeep - case "discard", "cache", "keep": - default: - return nil, errors.New("unknown config value, Migrations.Keep must be 'cache', 'pin', or 'discard'") - } - - if len(cfg.Migration.DownloadSources) == 0 { - cfg.Migration.DownloadSources = config.DefaultMigrationDownloadSources - } - - return &cfg.Migration, nil -} - -func readIpfsConfig(repoRoot *string) (bootstrap []string, peers []peer.AddrInfo) { - if repoRoot == nil { - return - } - - cfgPath, err := config.Filename(*repoRoot) - if err != nil { - fmt.Fprintln(os.Stderr, err) - return - } - - cfgFile, err := os.Open(cfgPath) - if err != nil { - fmt.Fprintln(os.Stderr, err) - return - } - defer cfgFile.Close() - - // Attempt to read bootstrap addresses - var bootstrapCfg struct { - Bootstrap []string - } - err = json.NewDecoder(cfgFile).Decode(&bootstrapCfg) - if err != nil { - fmt.Fprintln(os.Stderr, "cannot read bootstrap peers from config") - } else { - bootstrap = bootstrapCfg.Bootstrap - } - - if _, err = cfgFile.Seek(0, 0); err != nil { - fmt.Fprintln(os.Stderr, err) - } - - // Attempt to read peers - var peeringCfg struct { - Peering config.Peering - } - err = json.NewDecoder(cfgFile).Decode(&peeringCfg) - if err != nil { - fmt.Fprintln(os.Stderr, "cannot read peering from config") - } else { - peers = peeringCfg.Peering.Peers - } - - return -} - -// getMigrationFetcher creates one or more fetchers according to -// config.Migration.DownloadSources. If an IpfsFetcher is required, then -// bootstrap and peer information in read from the config file in repoRoot, -// unless repoRoot is nil. -func getMigrationFetcher(cfg *config.Migration, repoRoot *string) (migrations.Fetcher, error) { - const httpUserAgent = "go-ipfs" - - // Fetch migrations from current distribution, or location from environ - fetchDistPath := migrations.GetDistPathEnv(migrations.CurrentIpfsDist) - - var fetchers []migrations.Fetcher - for _, src := range cfg.DownloadSources { - src := strings.TrimSpace(src) - switch src { - case "IPFS", "ipfs": - bootstrap, peers := readIpfsConfig(repoRoot) - fetchers = append(fetchers, ipfsfetcher.NewIpfsFetcher(fetchDistPath, 0, bootstrap, peers)) - case "HTTPS", "https", "HTTP", "http": - fetchers = append(fetchers, migrations.NewHttpFetcher(fetchDistPath, "", httpUserAgent, 0)) - default: - u, err := url.Parse(src) - if err != nil { - return nil, fmt.Errorf("bad gateway address: %s", err) - } - switch u.Scheme { - case "": - u.Scheme = "https" - case "https", "http": - default: - return nil, errors.New("bad gateway address: url scheme must be http or https") - } - fetchers = append(fetchers, migrations.NewHttpFetcher(fetchDistPath, u.String(), httpUserAgent, 0)) - case "": - // Ignore empty string - } - } - if len(fetchers) == 0 { - return nil, errors.New("no sources specified") - } - - if len(fetchers) == 1 { - return fetchers[0], nil - } - - // Wrap fetchers in a MultiFetcher to try them in order - return migrations.NewMultiFetcher(fetchers...), nil -} - +// addMigrations adds any migration downloaded by the fetcher to the IPFS node func addMigrations(ctx context.Context, node *core.IpfsNode, fetcher migrations.Fetcher, pin bool) error { var fetchers []migrations.Fetcher if mf, ok := fetcher.(*migrations.MultiFetcher); ok { diff --git a/cmd/ipfs/daemon.go b/cmd/ipfs/daemon.go index f93152b316f..86fd5f9a5df 100644 --- a/cmd/ipfs/daemon.go +++ b/cmd/ipfs/daemon.go @@ -30,6 +30,7 @@ import ( nodeMount "github.com/ipfs/go-ipfs/fuse/node" fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo" "github.com/ipfs/go-ipfs/repo/fsrepo/migrations" + "github.com/ipfs/go-ipfs/repo/fsrepo/migrations/ipfsfetcher" sockets "github.com/libp2p/go-socket-activation" cmds "github.com/ipfs/go-ipfs-cmds" @@ -294,12 +295,22 @@ func daemonFunc(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment return fmt.Errorf("fs-repo requires migration") } - migrationCfg, err := readMigrationConfig(cctx.ConfigRoot) + // Read Migration section of IPFS config + migrationCfg, err := migrations.ReadMigrationConfig(cctx.ConfigRoot) if err != nil { return err } - fetcher, err = getMigrationFetcher(migrationCfg, &cctx.ConfigRoot) + // Define function to create IPFS fetcher + newIpfsFetcher := func(distPath string) migrations.Fetcher { + return ipfsfetcher.NewIpfsFetcher(distPath, 0, &cctx.ConfigRoot) + } + + // Fetch migrations from current distribution, or location from environ + fetchDistPath := migrations.GetDistPathEnv(migrations.CurrentIpfsDist) + + // Create fetchers according to migrationCfg.DownloadSources + fetcher, err = migrations.GetMigrationFetcher(migrationCfg.DownloadSources, fetchDistPath, newIpfsFetcher) if err != nil { return err } diff --git a/cmd/ipfs/migration_test.go b/cmd/ipfs/migration_test.go deleted file mode 100644 index da35bf3029c..00000000000 --- a/cmd/ipfs/migration_test.go +++ /dev/null @@ -1,312 +0,0 @@ -package main - -import ( - "io/ioutil" - "os" - "path/filepath" - "strings" - "testing" - - config "github.com/ipfs/go-ipfs-config" - "github.com/ipfs/go-ipfs/repo/fsrepo/migrations" - "github.com/ipfs/go-ipfs/repo/fsrepo/migrations/ipfsfetcher" -) - -var testConfig = ` -{ - "Bootstrap": [ - "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", - "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" - ], - "Migration": { - "DownloadSources": ["IPFS", "HTTP", "127.0.0.1", "https://127.0.1.1"], - "Keep": "cache" - }, - "Peering": { - "Peers": [ - { - "ID": "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5", - "Addrs": ["/ip4/127.0.0.1/tcp/4001", "/ip4/127.0.0.1/udp/4001/quic"] - } - ] - } -} -` - -func TestReadMigrationConfigDefaults(t *testing.T) { - tmpDir := makeConfig("{}") - defer os.RemoveAll(tmpDir) - - cfg, err := readMigrationConfig(tmpDir) - if err != nil { - t.Fatal(err) - } - - if cfg.Keep != config.DefaultMigrationKeep { - t.Error("expected default value for Keep") - } - - if len(cfg.DownloadSources) != len(config.DefaultMigrationDownloadSources) { - t.Fatal("expected default number of download sources") - } - for i, src := range config.DefaultMigrationDownloadSources { - if cfg.DownloadSources[i] != src { - t.Errorf("wrong DownloadSource: %s", cfg.DownloadSources[i]) - } - } -} - -func TestReadMigrationConfigErrors(t *testing.T) { - tmpDir := makeConfig(`{"Migration": {"Keep": "badvalue"}}`) - defer os.RemoveAll(tmpDir) - - _, err := readMigrationConfig(tmpDir) - if err == nil { - t.Fatal("expected error") - } - if !strings.HasPrefix(err.Error(), "unknown") { - t.Fatal("did not get expected error:", err) - } - - os.RemoveAll(tmpDir) - _, err = readMigrationConfig(tmpDir) - if err == nil { - t.Fatal("expected error") - } - - bootstrap, peers := readIpfsConfig(&tmpDir) - if bootstrap != nil { - t.Error("expected nil bootstrap") - } - if peers != nil { - t.Error("expected nil peers") - } - - tmpDir = makeConfig(`}{`) - defer os.RemoveAll(tmpDir) - _, err = readMigrationConfig(tmpDir) - if err == nil { - t.Fatal("expected error") - } -} - -func TestReadMigrationConfig(t *testing.T) { - tmpDir := makeConfig(testConfig) - defer os.RemoveAll(tmpDir) - - cfg, err := readMigrationConfig(tmpDir) - if err != nil { - t.Fatal(err) - } - - if len(cfg.DownloadSources) != 4 { - t.Fatal("wrong number of DownloadSources") - } - expect := []string{"IPFS", "HTTP", "127.0.0.1", "https://127.0.1.1"} - for i := range expect { - if cfg.DownloadSources[i] != expect[i] { - t.Errorf("wrong DownloadSource at %d", i) - } - } - - if cfg.Keep != "cache" { - t.Error("wrong value for Keep") - } -} - -func TestReadIpfsConfig(t *testing.T) { - tmpDir := makeConfig(testConfig) - defer os.RemoveAll(tmpDir) - - bootstrap, peers := readIpfsConfig(nil) - if bootstrap != nil || peers != nil { - t.Fatal("expected nil ipfs config items") - } - - bootstrap, peers = readIpfsConfig(&tmpDir) - if len(bootstrap) != 2 { - t.Fatal("wrong number of bootstrap addresses") - } - if bootstrap[0] != "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt" { - t.Fatal("wrong bootstrap address") - } - - if len(peers) != 1 { - t.Fatal("wrong number of peers") - } - - peer := peers[0] - if peer.ID.String() != "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5" { - t.Errorf("wrong ID for first peer") - } - if len(peer.Addrs) != 2 { - t.Error("wrong number of addrs for first peer") - } -} - -func TestReadPartialIpfsConfig(t *testing.T) { - const ( - configBadBootstrap = ` -{ - "Bootstrap": "unreadable", - "Migration": { - "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], - "Keep": "cache" - }, - "Peering": { - "Peers": [ - { - "ID": "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5", - "Addrs": ["/ip4/127.0.0.1/tcp/4001", "/ip4/127.0.0.1/udp/4001/quic"] - } - ] - } -} -` - configBadPeers = ` -{ - "Bootstrap": [ - "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", - "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" - ], - "Migration": { - "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], - "Keep": "cache" - }, - "Peering": "Unreadable-data" -} -` - ) - - tmpDir := makeConfig(configBadBootstrap) - defer os.RemoveAll(tmpDir) - - bootstrap, peers := readIpfsConfig(&tmpDir) - if bootstrap != nil { - t.Fatal("expected nil bootstrap") - } - if len(peers) != 1 { - t.Fatal("wrong number of peers") - } - if len(peers[0].Addrs) != 2 { - t.Error("wrong number of addrs for first peer") - } - os.RemoveAll(tmpDir) - - tmpDir = makeConfig(configBadPeers) - defer os.RemoveAll(tmpDir) - - bootstrap, peers = readIpfsConfig(&tmpDir) - if peers != nil { - t.Fatal("expected nil peers") - } - if len(bootstrap) != 2 { - t.Fatal("wrong number of bootstrap addresses") - } - if bootstrap[0] != "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt" { - t.Fatal("wrong bootstrap address") - } -} - -func makeConfig(configData string) string { - tmpDir, err := ioutil.TempDir("", "migration_test") - if err != nil { - panic(err) - } - - cfgFile, err := os.Create(filepath.Join(tmpDir, "config")) - if err != nil { - panic(err) - } - if _, err = cfgFile.Write([]byte(configData)); err != nil { - panic(err) - } - if err = cfgFile.Close(); err != nil { - panic(err) - } - return tmpDir -} - -func TestGetMigrationFetcher(t *testing.T) { - var f migrations.Fetcher - var err error - - cfg := &config.Migration{} - - cfg.DownloadSources = []string{"ftp://bad.gateway.io"} - _, err = getMigrationFetcher(cfg, nil) - if err == nil || !strings.HasPrefix(err.Error(), "bad gateway addr") { - t.Fatal("Expected bad gateway address error, got:", err) - } - - cfg.DownloadSources = []string{"::bad.gateway.io"} - _, err = getMigrationFetcher(cfg, nil) - if err == nil || !strings.HasPrefix(err.Error(), "bad gateway addr") { - t.Fatal("Expected bad gateway address error, got:", err) - } - - cfg.DownloadSources = []string{"http://localhost"} - f, err = getMigrationFetcher(cfg, nil) - if err != nil { - t.Fatal(err) - } - if _, ok := f.(*migrations.HttpFetcher); !ok { - t.Fatal("expected HttpFetcher") - } - - cfg.DownloadSources = []string{"ipfs"} - f, err = getMigrationFetcher(cfg, nil) - if err != nil { - t.Fatal(err) - } - if _, ok := f.(*ipfsfetcher.IpfsFetcher); !ok { - t.Fatal("expected IpfsFetcher") - } - - cfg.DownloadSources = []string{"http"} - f, err = getMigrationFetcher(cfg, nil) - if err != nil { - t.Fatal(err) - } - if _, ok := f.(*migrations.HttpFetcher); !ok { - t.Fatal("expected HttpFetcher") - } - - cfg.DownloadSources = []string{"IPFS", "HTTPS"} - f, err = getMigrationFetcher(cfg, nil) - if err != nil { - t.Fatal(err) - } - mf, ok := f.(*migrations.MultiFetcher) - if !ok { - t.Fatal("expected MultiFetcher") - } - if mf.Len() != 2 { - t.Fatal("expected 2 fetchers in MultiFetcher") - } - - cfg.DownloadSources = []string{"ipfs", "https", "some.domain.io"} - f, err = getMigrationFetcher(cfg, nil) - if err != nil { - t.Fatal(err) - } - mf, ok = f.(*migrations.MultiFetcher) - if !ok { - t.Fatal("expected MultiFetcher") - } - if mf.Len() != 3 { - t.Fatal("expected 3 fetchers in MultiFetcher") - } - - cfg.DownloadSources = nil - _, err = getMigrationFetcher(cfg, nil) - if err == nil { - t.Fatal("expected error when no sources specified") - } - - cfg.DownloadSources = []string{"", ""} - _, err = getMigrationFetcher(cfg, nil) - if err == nil { - t.Fatal("expected error when empty string fetchers specified") - } -} diff --git a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go index 0a133a83311..40740ba3ebc 100644 --- a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go +++ b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go @@ -2,6 +2,7 @@ package ipfsfetcher import ( "context" + "encoding/json" "fmt" "io" "io/ioutil" @@ -32,10 +33,9 @@ const ( ) type IpfsFetcher struct { - distPath string - limit int64 - bootstrap []string - peers []peer.AddrInfo + distPath string + limit int64 + repoRoot *string openOnce sync.Once openErr error @@ -56,12 +56,15 @@ type IpfsFetcher struct { // // Specifying "" for distPath sets the default IPNS path. // Specifying 0 for fetchLimit sets the default, -1 means no limit. -func NewIpfsFetcher(distPath string, fetchLimit int64, bootstrap []string, peers []peer.AddrInfo) *IpfsFetcher { +// +// Bootstrap and peer information in read from the IPFS config file in +// repoRoot, unless repoRoot is nil. If repoRoot is empty (""), then read the +// config from the default IPFS directory. +func NewIpfsFetcher(distPath string, fetchLimit int64, repoRoot *string) *IpfsFetcher { f := &IpfsFetcher{ - limit: defaultFetchLimit, - distPath: migrations.LatestIpfsDist, - bootstrap: bootstrap, - peers: peers, + limit: defaultFetchLimit, + distPath: migrations.LatestIpfsDist, + repoRoot: repoRoot, } if distPath != "" { @@ -88,7 +91,8 @@ func (f *IpfsFetcher) Fetch(ctx context.Context, filePath string) (io.ReadCloser // Initialize and start IPFS node on first call to Fetch, since the fetcher // may be created by not used. f.openOnce.Do(func() { - f.ipfsTmpDir, f.openErr = initTempNode(ctx, f.bootstrap, f.peers) + bootstrap, peers := readIpfsConfig(f.repoRoot) + f.ipfsTmpDir, f.openErr = initTempNode(ctx, bootstrap, peers) if f.openErr != nil { return } @@ -277,3 +281,50 @@ func parsePath(fetchPath string) (ipath.Path, error) { } return ipfsPath, ipfsPath.IsValid() } + +func readIpfsConfig(repoRoot *string) (bootstrap []string, peers []peer.AddrInfo) { + if repoRoot == nil { + return + } + + cfgPath, err := config.Filename(*repoRoot) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return + } + + cfgFile, err := os.Open(cfgPath) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return + } + defer cfgFile.Close() + + // Attempt to read bootstrap addresses + var bootstrapCfg struct { + Bootstrap []string + } + err = json.NewDecoder(cfgFile).Decode(&bootstrapCfg) + if err != nil { + fmt.Fprintln(os.Stderr, "cannot read bootstrap peers from config") + } else { + bootstrap = bootstrapCfg.Bootstrap + } + + if _, err = cfgFile.Seek(0, 0); err != nil { + fmt.Fprintln(os.Stderr, err) + } + + // Attempt to read peers + var peeringCfg struct { + Peering config.Peering + } + err = json.NewDecoder(cfgFile).Decode(&peeringCfg) + if err != nil { + fmt.Fprintln(os.Stderr, "cannot read peering from config") + } else { + peers = peeringCfg.Peering.Peers + } + + return +} diff --git a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go index 4c4d5b2d662..8e6ed27eb5a 100644 --- a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go +++ b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "fmt" + "io/ioutil" "os" "path/filepath" "testing" @@ -25,7 +26,7 @@ func TestIpfsFetcher(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - fetcher := NewIpfsFetcher("", 0, nil, nil) + fetcher := NewIpfsFetcher("", 0, nil) defer fetcher.Close() rc, err := fetcher.Fetch(ctx, "go-ipfs/versions") @@ -63,11 +64,11 @@ func TestInitIpfsFetcher(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - f := NewIpfsFetcher("", 0, nil, nil) + f := NewIpfsFetcher("", 0, nil) defer f.Close() // Init ipfs repo - f.ipfsTmpDir, f.openErr = initTempNode(ctx, f.bootstrap, f.peers) + f.ipfsTmpDir, f.openErr = initTempNode(ctx, nil, nil) if f.openErr != nil { t.Fatalf("failed to initialize ipfs node: %s", f.openErr) } @@ -110,6 +111,149 @@ func TestInitIpfsFetcher(t *testing.T) { } } +func TestReadIpfsConfig(t *testing.T) { + var testConfig = ` +{ + "Bootstrap": [ + "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", + "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" + ], + "Migration": { + "DownloadSources": ["IPFS", "HTTP", "127.0.0.1", "https://127.0.1.1"], + "Keep": "cache" + }, + "Peering": { + "Peers": [ + { + "ID": "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5", + "Addrs": ["/ip4/127.0.0.1/tcp/4001", "/ip4/127.0.0.1/udp/4001/quic"] + } + ] + } +} +` + + noSuchDir := "no_such_dir-5953aa51-1145-4efd-afd1-a069075fcf76" + bootstrap, peers := readIpfsConfig(&noSuchDir) + if bootstrap != nil { + t.Error("expected nil bootstrap") + } + if peers != nil { + t.Error("expected nil peers") + } + + tmpDir := makeConfig(testConfig) + defer os.RemoveAll(tmpDir) + + bootstrap, peers = readIpfsConfig(nil) + if bootstrap != nil || peers != nil { + t.Fatal("expected nil ipfs config items") + } + + bootstrap, peers = readIpfsConfig(&tmpDir) + if len(bootstrap) != 2 { + t.Fatal("wrong number of bootstrap addresses") + } + if bootstrap[0] != "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt" { + t.Fatal("wrong bootstrap address") + } + + if len(peers) != 1 { + t.Fatal("wrong number of peers") + } + + peer := peers[0] + if peer.ID.String() != "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5" { + t.Errorf("wrong ID for first peer") + } + if len(peer.Addrs) != 2 { + t.Error("wrong number of addrs for first peer") + } +} + +func TestReadPartialIpfsConfig(t *testing.T) { + const ( + configBadBootstrap = ` +{ + "Bootstrap": "unreadable", + "Migration": { + "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], + "Keep": "cache" + }, + "Peering": { + "Peers": [ + { + "ID": "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5", + "Addrs": ["/ip4/127.0.0.1/tcp/4001", "/ip4/127.0.0.1/udp/4001/quic"] + } + ] + } +} +` + configBadPeers = ` +{ + "Bootstrap": [ + "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", + "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" + ], + "Migration": { + "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], + "Keep": "cache" + }, + "Peering": "Unreadable-data" +} +` + ) + + tmpDir := makeConfig(configBadBootstrap) + defer os.RemoveAll(tmpDir) + + bootstrap, peers := readIpfsConfig(&tmpDir) + if bootstrap != nil { + t.Fatal("expected nil bootstrap") + } + if len(peers) != 1 { + t.Fatal("wrong number of peers") + } + if len(peers[0].Addrs) != 2 { + t.Error("wrong number of addrs for first peer") + } + os.RemoveAll(tmpDir) + + tmpDir = makeConfig(configBadPeers) + defer os.RemoveAll(tmpDir) + + bootstrap, peers = readIpfsConfig(&tmpDir) + if peers != nil { + t.Fatal("expected nil peers") + } + if len(bootstrap) != 2 { + t.Fatal("wrong number of bootstrap addresses") + } + if bootstrap[0] != "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt" { + t.Fatal("wrong bootstrap address") + } +} + +func makeConfig(configData string) string { + tmpDir, err := ioutil.TempDir("", "migration_test") + if err != nil { + panic(err) + } + + cfgFile, err := os.Create(filepath.Join(tmpDir, "config")) + if err != nil { + panic(err) + } + if _, err = cfgFile.Write([]byte(configData)); err != nil { + panic(err) + } + if err = cfgFile.Close(); err != nil { + panic(err) + } + return tmpDir +} + func skipUnlessEpic(t *testing.T) { if os.Getenv("IPFS_EPIC_TEST") == "" { t.SkipNow() diff --git a/repo/fsrepo/migrations/migrations.go b/repo/fsrepo/migrations/migrations.go index 2cb13619aef..4e4de6096ed 100644 --- a/repo/fsrepo/migrations/migrations.go +++ b/repo/fsrepo/migrations/migrations.go @@ -2,15 +2,20 @@ package migrations import ( "context" + "encoding/json" + "errors" "fmt" "io/ioutil" "log" + "net/url" "os" "os/exec" "path" "runtime" "strings" "sync" + + config "github.com/ipfs/go-ipfs-config" ) const ( @@ -107,6 +112,89 @@ func ExeName(name string) string { return name } +// ReadMigrationConfig reads the Migration section of the IPFS config, avoiding +// reading anything other than the Migration section. That way, we're free to +// make arbitrary changes to all _other_ sections in migrations. +func ReadMigrationConfig(repoRoot string) (*config.Migration, error) { + var cfg struct { + Migration config.Migration + } + + cfgPath, err := config.Filename(repoRoot) + if err != nil { + return nil, err + } + + cfgFile, err := os.Open(cfgPath) + if err != nil { + return nil, err + } + defer cfgFile.Close() + + err = json.NewDecoder(cfgFile).Decode(&cfg) + if err != nil { + return nil, err + } + + switch cfg.Migration.Keep { + case "": + cfg.Migration.Keep = config.DefaultMigrationKeep + case "discard", "cache", "keep": + default: + return nil, errors.New("unknown config value, Migrations.Keep must be 'cache', 'pin', or 'discard'") + } + + if len(cfg.Migration.DownloadSources) == 0 { + cfg.Migration.DownloadSources = config.DefaultMigrationDownloadSources + } + + return &cfg.Migration, nil +} + +// GetMigrationFetcher creates one or more fetchers according to +// downloadSources, +func GetMigrationFetcher(downloadSources []string, distPath string, newIpfsFetcher func(string) Fetcher) (Fetcher, error) { + const httpUserAgent = "go-ipfs" + + var fetchers []Fetcher + for _, src := range downloadSources { + src := strings.TrimSpace(src) + switch src { + case "HTTPS", "https", "HTTP", "http": + fetchers = append(fetchers, NewHttpFetcher(distPath, "", httpUserAgent, 0)) + case "IPFS", "ipfs": + if newIpfsFetcher != nil { + fetchers = append(fetchers, newIpfsFetcher(distPath)) + } + default: + u, err := url.Parse(src) + if err != nil { + return nil, fmt.Errorf("bad gateway address: %s", err) + } + switch u.Scheme { + case "": + u.Scheme = "https" + case "https", "http": + default: + return nil, errors.New("bad gateway address: url scheme must be http or https") + } + fetchers = append(fetchers, NewHttpFetcher(distPath, u.String(), httpUserAgent, 0)) + case "": + // Ignore empty string + } + } + if len(fetchers) == 0 { + return nil, errors.New("no sources specified") + } + + if len(fetchers) == 1 { + return fetchers[0], nil + } + + // Wrap fetchers in a MultiFetcher to try them in order + return NewMultiFetcher(fetchers...), nil +} + func migrationName(from, to int) string { return fmt.Sprintf("fs-repo-%d-to-%d", from, to) } diff --git a/repo/fsrepo/migrations/migrations_test.go b/repo/fsrepo/migrations/migrations_test.go index 66827d8c749..dee98f62845 100644 --- a/repo/fsrepo/migrations/migrations_test.go +++ b/repo/fsrepo/migrations/migrations_test.go @@ -3,12 +3,15 @@ package migrations import ( "context" "fmt" + "io" "io/ioutil" "log" "os" "path/filepath" "strings" "testing" + + config "github.com/ipfs/go-ipfs-config" ) func TestFindMigrations(t *testing.T) { @@ -211,3 +214,212 @@ func createFakeBin(from, to int, tmpDir string) { panic(err) } } + +var testConfig = ` +{ + "Bootstrap": [ + "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", + "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" + ], + "Migration": { + "DownloadSources": ["IPFS", "HTTP", "127.0.0.1", "https://127.0.1.1"], + "Keep": "cache" + }, + "Peering": { + "Peers": [ + { + "ID": "12D3KooWGC6TvWhfapngX6wvJHMYvKpDMXPb3ZnCZ6dMoaMtimQ5", + "Addrs": ["/ip4/127.0.0.1/tcp/4001", "/ip4/127.0.0.1/udp/4001/quic"] + } + ] + } +} +` + +func TestReadMigrationConfigDefaults(t *testing.T) { + tmpDir := makeConfig("{}") + defer os.RemoveAll(tmpDir) + + cfg, err := ReadMigrationConfig(tmpDir) + if err != nil { + t.Fatal(err) + } + + if cfg.Keep != config.DefaultMigrationKeep { + t.Error("expected default value for Keep") + } + + if len(cfg.DownloadSources) != len(config.DefaultMigrationDownloadSources) { + t.Fatal("expected default number of download sources") + } + for i, src := range config.DefaultMigrationDownloadSources { + if cfg.DownloadSources[i] != src { + t.Errorf("wrong DownloadSource: %s", cfg.DownloadSources[i]) + } + } +} + +func TestReadMigrationConfigErrors(t *testing.T) { + tmpDir := makeConfig(`{"Migration": {"Keep": "badvalue"}}`) + defer os.RemoveAll(tmpDir) + + _, err := ReadMigrationConfig(tmpDir) + if err == nil { + t.Fatal("expected error") + } + if !strings.HasPrefix(err.Error(), "unknown") { + t.Fatal("did not get expected error:", err) + } + + os.RemoveAll(tmpDir) + _, err = ReadMigrationConfig(tmpDir) + if err == nil { + t.Fatal("expected error") + } + + tmpDir = makeConfig(`}{`) + defer os.RemoveAll(tmpDir) + _, err = ReadMigrationConfig(tmpDir) + if err == nil { + t.Fatal("expected error") + } +} + +func TestReadMigrationConfig(t *testing.T) { + tmpDir := makeConfig(testConfig) + defer os.RemoveAll(tmpDir) + + cfg, err := ReadMigrationConfig(tmpDir) + if err != nil { + t.Fatal(err) + } + + if len(cfg.DownloadSources) != 4 { + t.Fatal("wrong number of DownloadSources") + } + expect := []string{"IPFS", "HTTP", "127.0.0.1", "https://127.0.1.1"} + for i := range expect { + if cfg.DownloadSources[i] != expect[i] { + t.Errorf("wrong DownloadSource at %d", i) + } + } + + if cfg.Keep != "cache" { + t.Error("wrong value for Keep") + } +} + +type mockIpfsFetcher struct{} + +func (m *mockIpfsFetcher) Fetch(ctx context.Context, filePath string) (io.ReadCloser, error) { + return nil, nil +} + +func (m *mockIpfsFetcher) Close() error { + return nil +} + +func TestGetMigrationFetcher(t *testing.T) { + var f Fetcher + var err error + + newIpfsFetcher := func(distPath string) Fetcher { + return &mockIpfsFetcher{} + } + + downloadSources := []string{"ftp://bad.gateway.io"} + _, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err == nil || !strings.HasPrefix(err.Error(), "bad gateway addr") { + t.Fatal("Expected bad gateway address error, got:", err) + } + + downloadSources = []string{"::bad.gateway.io"} + _, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err == nil || !strings.HasPrefix(err.Error(), "bad gateway addr") { + t.Fatal("Expected bad gateway address error, got:", err) + } + + downloadSources = []string{"http://localhost"} + f, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err != nil { + t.Fatal(err) + } + if _, ok := f.(*HttpFetcher); !ok { + t.Fatal("expected HttpFetcher") + } + + downloadSources = []string{"ipfs"} + f, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err != nil { + t.Fatal(err) + } + if _, ok := f.(*mockIpfsFetcher); !ok { + t.Fatal("expected IpfsFetcher") + } + + downloadSources = []string{"http"} + f, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err != nil { + t.Fatal(err) + } + if _, ok := f.(*HttpFetcher); !ok { + t.Fatal("expected HttpFetcher") + } + + downloadSources = []string{"IPFS", "HTTPS"} + f, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err != nil { + t.Fatal(err) + } + mf, ok := f.(*MultiFetcher) + if !ok { + t.Fatal("expected MultiFetcher") + } + if mf.Len() != 2 { + t.Fatal("expected 2 fetchers in MultiFetcher") + } + + downloadSources = []string{"ipfs", "https", "some.domain.io"} + f, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err != nil { + t.Fatal(err) + } + mf, ok = f.(*MultiFetcher) + if !ok { + t.Fatal("expected MultiFetcher") + } + if mf.Len() != 3 { + t.Fatal("expected 3 fetchers in MultiFetcher") + } + + downloadSources = nil + _, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err == nil { + t.Fatal("expected error when no sources specified") + } + + downloadSources = []string{"", ""} + _, err = GetMigrationFetcher(downloadSources, "", newIpfsFetcher) + if err == nil { + t.Fatal("expected error when empty string fetchers specified") + } +} + +func makeConfig(configData string) string { + tmpDir, err := ioutil.TempDir("", "migration_test") + if err != nil { + panic(err) + } + + cfgFile, err := os.Create(filepath.Join(tmpDir, "config")) + if err != nil { + panic(err) + } + if _, err = cfgFile.Write([]byte(configData)); err != nil { + panic(err) + } + if err = cfgFile.Close(); err != nil { + panic(err) + } + return tmpDir +} From 1ca7b762a012054c07854dad53858f76260a63a6 Mon Sep 17 00:00:00 2001 From: gammazero Date: Thu, 24 Jun 2021 13:31:48 -0700 Subject: [PATCH 2/3] Add comment --- cmd/ipfs/daemon.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/ipfs/daemon.go b/cmd/ipfs/daemon.go index 86fd5f9a5df..f430fcdd272 100644 --- a/cmd/ipfs/daemon.go +++ b/cmd/ipfs/daemon.go @@ -301,7 +301,11 @@ func daemonFunc(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment return err } - // Define function to create IPFS fetcher + // Define function to create IPFS fetcher. Do not supply an + // already-constructed IPFS fetcher, because this may be expensive and + // not needed according to migration config. Instead, supply a function + // to construct the particular IPFS fetcher implementation used here, + // which is called only if an IPFS fetcher is needed. newIpfsFetcher := func(distPath string) migrations.Fetcher { return ipfsfetcher.NewIpfsFetcher(distPath, 0, &cctx.ConfigRoot) } From b42c02236531c51b0295f0db6e8b7b1a9c2a4c0e Mon Sep 17 00:00:00 2001 From: gammazero Date: Tue, 27 Jul 2021 14:31:35 -0700 Subject: [PATCH 3/3] Review changes --- repo/fsrepo/migrations/fetch_test.go | 6 +- repo/fsrepo/migrations/ipfsdir_test.go | 8 +-- .../migrations/ipfsfetcher/ipfsfetcher.go | 2 + .../ipfsfetcher/ipfsfetcher_test.go | 60 +++++++++---------- repo/fsrepo/migrations/migrations.go | 8 +-- repo/fsrepo/migrations/migrations_test.go | 52 +++++----------- repo/fsrepo/migrations/unpack_test.go | 16 ++--- 7 files changed, 53 insertions(+), 99 deletions(-) diff --git a/repo/fsrepo/migrations/fetch_test.go b/repo/fsrepo/migrations/fetch_test.go index c78063da0f5..ec7c6d5e73e 100644 --- a/repo/fsrepo/migrations/fetch_test.go +++ b/repo/fsrepo/migrations/fetch_test.go @@ -127,11 +127,7 @@ func TestHttpFetch(t *testing.T) { } func TestFetchBinary(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "fetchtest") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() ctx, cancel := context.WithCancel(context.Background()) defer cancel() diff --git a/repo/fsrepo/migrations/ipfsdir_test.go b/repo/fsrepo/migrations/ipfsdir_test.go index 2fecefa9216..ff4a19cda92 100644 --- a/repo/fsrepo/migrations/ipfsdir_test.go +++ b/repo/fsrepo/migrations/ipfsdir_test.go @@ -13,13 +13,7 @@ var ( ) func TestRepoDir(t *testing.T) { - var err error - fakeHome, err = ioutil.TempDir("", "testhome") - if err != nil { - panic(err) - } - defer os.RemoveAll(fakeHome) - + fakeHome = t.TempDir() os.Setenv("HOME", fakeHome) fakeIpfs = filepath.Join(fakeHome, ".ipfs") diff --git a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go index 40740ba3ebc..88f07b502ee 100644 --- a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go +++ b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher.go @@ -312,6 +312,8 @@ func readIpfsConfig(repoRoot *string) (bootstrap []string, peers []peer.AddrInfo } if _, err = cfgFile.Seek(0, 0); err != nil { + // If Seek fails, only log the error and continue on to try to read the + // peering config anyway as it might still be readable fmt.Fprintln(os.Stderr, err) } diff --git a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go index 8e6ed27eb5a..877de5e788e 100644 --- a/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go +++ b/repo/fsrepo/migrations/ipfsfetcher/ipfsfetcher_test.go @@ -4,7 +4,6 @@ import ( "bufio" "context" "fmt" - "io/ioutil" "os" "path/filepath" "testing" @@ -142,8 +141,7 @@ func TestReadIpfsConfig(t *testing.T) { t.Error("expected nil peers") } - tmpDir := makeConfig(testConfig) - defer os.RemoveAll(tmpDir) + tmpDir := makeConfig(t, testConfig) bootstrap, peers = readIpfsConfig(nil) if bootstrap != nil || peers != nil { @@ -171,9 +169,8 @@ func TestReadIpfsConfig(t *testing.T) { } } -func TestReadPartialIpfsConfig(t *testing.T) { - const ( - configBadBootstrap = ` +func TestBadBootstrappingIpfsConfig(t *testing.T) { + const configBadBootstrap = ` { "Bootstrap": "unreadable", "Migration": { @@ -190,23 +187,8 @@ func TestReadPartialIpfsConfig(t *testing.T) { } } ` - configBadPeers = ` -{ - "Bootstrap": [ - "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", - "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" - ], - "Migration": { - "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], - "Keep": "cache" - }, - "Peering": "Unreadable-data" -} -` - ) - tmpDir := makeConfig(configBadBootstrap) - defer os.RemoveAll(tmpDir) + tmpDir := makeConfig(t, configBadBootstrap) bootstrap, peers := readIpfsConfig(&tmpDir) if bootstrap != nil { @@ -219,11 +201,26 @@ func TestReadPartialIpfsConfig(t *testing.T) { t.Error("wrong number of addrs for first peer") } os.RemoveAll(tmpDir) +} - tmpDir = makeConfig(configBadPeers) - defer os.RemoveAll(tmpDir) +func TestBadPeersIpfsConfig(t *testing.T) { + const configBadPeers = ` +{ + "Bootstrap": [ + "/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt", + "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ" + ], + "Migration": { + "DownloadSources": ["IPFS", "HTTP", "127.0.0.1"], + "Keep": "cache" + }, + "Peering": "Unreadable-data" +} +` - bootstrap, peers = readIpfsConfig(&tmpDir) + tmpDir := makeConfig(t, configBadPeers) + + bootstrap, peers := readIpfsConfig(&tmpDir) if peers != nil { t.Fatal("expected nil peers") } @@ -235,21 +232,18 @@ func TestReadPartialIpfsConfig(t *testing.T) { } } -func makeConfig(configData string) string { - tmpDir, err := ioutil.TempDir("", "migration_test") - if err != nil { - panic(err) - } +func makeConfig(t *testing.T, configData string) string { + tmpDir := t.TempDir() cfgFile, err := os.Create(filepath.Join(tmpDir, "config")) if err != nil { - panic(err) + t.Fatal(err) } if _, err = cfgFile.Write([]byte(configData)); err != nil { - panic(err) + t.Fatal(err) } if err = cfgFile.Close(); err != nil { - panic(err) + t.Fatal(err) } return tmpDir } diff --git a/repo/fsrepo/migrations/migrations.go b/repo/fsrepo/migrations/migrations.go index 4e4de6096ed..861dc7b7caf 100644 --- a/repo/fsrepo/migrations/migrations.go +++ b/repo/fsrepo/migrations/migrations.go @@ -183,11 +183,11 @@ func GetMigrationFetcher(downloadSources []string, distPath string, newIpfsFetch // Ignore empty string } } - if len(fetchers) == 0 { - return nil, errors.New("no sources specified") - } - if len(fetchers) == 1 { + switch len(fetchers) { + case 0: + return nil, errors.New("no sources specified") + case 1: return fetchers[0], nil } diff --git a/repo/fsrepo/migrations/migrations_test.go b/repo/fsrepo/migrations/migrations_test.go index dee98f62845..b09174a4930 100644 --- a/repo/fsrepo/migrations/migrations_test.go +++ b/repo/fsrepo/migrations/migrations_test.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "io/ioutil" "log" "os" "path/filepath" @@ -15,11 +14,7 @@ import ( ) func TestFindMigrations(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "migratetest") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -66,11 +61,7 @@ func TestFindMigrations(t *testing.T) { } func TestFindMigrationsReverse(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "migratetest") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -124,11 +115,7 @@ func TestFetchMigrations(t *testing.T) { defer ts.Close() fetcher := NewHttpFetcher(CurrentIpfsDist, ts.URL, "", 0) - tmpDir, err := ioutil.TempDir("", "migratetest") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() needed := []string{"fs-repo-1-to-2", "fs-repo-2-to-3"} buf := new(strings.Builder) @@ -160,16 +147,12 @@ func TestFetchMigrations(t *testing.T) { } func TestRunMigrations(t *testing.T) { - fakeHome, err := ioutil.TempDir("", "testhome") - if err != nil { - panic(err) - } - defer os.RemoveAll(fakeHome) + fakeHome := t.TempDir() os.Setenv("HOME", fakeHome) fakeIpfs := filepath.Join(fakeHome, ".ipfs") - err = os.Mkdir(fakeIpfs, os.ModePerm) + err := os.Mkdir(fakeIpfs, os.ModePerm) if err != nil { panic(err) } @@ -237,8 +220,7 @@ var testConfig = ` ` func TestReadMigrationConfigDefaults(t *testing.T) { - tmpDir := makeConfig("{}") - defer os.RemoveAll(tmpDir) + tmpDir := makeConfig(t, "{}") cfg, err := ReadMigrationConfig(tmpDir) if err != nil { @@ -260,8 +242,7 @@ func TestReadMigrationConfigDefaults(t *testing.T) { } func TestReadMigrationConfigErrors(t *testing.T) { - tmpDir := makeConfig(`{"Migration": {"Keep": "badvalue"}}`) - defer os.RemoveAll(tmpDir) + tmpDir := makeConfig(t, `{"Migration": {"Keep": "badvalue"}}`) _, err := ReadMigrationConfig(tmpDir) if err == nil { @@ -277,8 +258,7 @@ func TestReadMigrationConfigErrors(t *testing.T) { t.Fatal("expected error") } - tmpDir = makeConfig(`}{`) - defer os.RemoveAll(tmpDir) + tmpDir = makeConfig(t, `}{`) _, err = ReadMigrationConfig(tmpDir) if err == nil { t.Fatal("expected error") @@ -286,8 +266,7 @@ func TestReadMigrationConfigErrors(t *testing.T) { } func TestReadMigrationConfig(t *testing.T) { - tmpDir := makeConfig(testConfig) - defer os.RemoveAll(tmpDir) + tmpDir := makeConfig(t, testConfig) cfg, err := ReadMigrationConfig(tmpDir) if err != nil { @@ -405,21 +384,18 @@ func TestGetMigrationFetcher(t *testing.T) { } } -func makeConfig(configData string) string { - tmpDir, err := ioutil.TempDir("", "migration_test") - if err != nil { - panic(err) - } +func makeConfig(t *testing.T, configData string) string { + tmpDir := t.TempDir() cfgFile, err := os.Create(filepath.Join(tmpDir, "config")) if err != nil { - panic(err) + t.Fatal(err) } if _, err = cfgFile.Write([]byte(configData)); err != nil { - panic(err) + t.Fatal(err) } if err = cfgFile.Close(); err != nil { - panic(err) + t.Fatal(err) } return tmpDir } diff --git a/repo/fsrepo/migrations/unpack_test.go b/repo/fsrepo/migrations/unpack_test.go index f4266b0246f..161ababbf47 100644 --- a/repo/fsrepo/migrations/unpack_test.go +++ b/repo/fsrepo/migrations/unpack_test.go @@ -33,14 +33,10 @@ func TestUnpackArchive(t *testing.T) { } func TestUnpackTgz(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "testunpacktgz") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() badTarGzip := filepath.Join(tmpDir, "bad.tar.gz") - err = ioutil.WriteFile(badTarGzip, []byte("bad-data\n"), 0644) + err := ioutil.WriteFile(badTarGzip, []byte("bad-data\n"), 0644) if err != nil { panic(err) } @@ -81,14 +77,10 @@ func TestUnpackTgz(t *testing.T) { } func TestUnpackZip(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "testunpackzip") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() badZip := filepath.Join(tmpDir, "bad.zip") - err = ioutil.WriteFile(badZip, []byte("bad-data\n"), 0644) + err := ioutil.WriteFile(badZip, []byte("bad-data\n"), 0644) if err != nil { panic(err) }