Merge pull request #1587 from aptly-dev/fix/check-storage-exists

publish: check if storage exists

Author: neolynx

api/files.go

@@ -230,7 +230,7 @@ func apiFilesUploadOne(c *gin.Context) {
 		AbortWithJSONError(c, 500, err)
 		return
 	}
-	defer dst.Close()
+	defer func() { _ = dst.Close() }()
 
 	if _, err = io.Copy(dst, c.Request.Body); err != nil {
 		AbortWithJSONError(c, 500, err)

api/published_file_missing_test.go

@@ -220,7 +220,8 @@ func (s *PublishedFileMissingSuite) TestPublishedFileGoMissing(c *C) {
 	c.Assert(resp.Code, Equals, 200, Commentf("Failed to update publish: %s", resp.Body.String()))
 
 	// Now check if the file is actually accessible in the published location
-	publishedStorage := s.context.GetPublishedStorage("")
+	publishedStorage, err := s.context.GetPublishedStorage("")
+	c.Assert(err, IsNil)
 	publicPath := publishedStorage.(aptly.FileSystemPublishedStorage).PublicPath()
 
 	// Expected file path: hrt/pool/main/h/hrt-libblobbyclient1/hrt-libblobbyclient1_20250926.152427+hrtdeb11_amd64.deb
@@ -332,7 +333,8 @@ func (s *PublishedFileMissingSuite) TestConcurrentPublishRace(c *C) {
 		c.Assert(err, IsNil)
 
 		// Check published files
-		publishedStorage := s.context.GetPublishedStorage("")
+		publishedStorage, err := s.context.GetPublishedStorage("")
+		c.Assert(err, IsNil)
 		publicPath := publishedStorage.(aptly.FileSystemPublishedStorage).PublicPath()
 
 		missingFiles := []string{}
@@ -446,7 +448,8 @@ func (s *PublishedFileMissingSuite) TestIdenticalPackageRace(c *C) {
 		c.Logf("[iter %d] All operations complete", iter)
 
 		// Check the shared pool location
-		publishedStorage := s.context.GetPublishedStorage("")
+		publishedStorage, err := s.context.GetPublishedStorage("")
+		c.Assert(err, IsNil)
 		publicPath := publishedStorage.(aptly.FileSystemPublishedStorage).PublicPath()
 
 		poolSubdir := string(packageName[0])
@@ -663,7 +666,8 @@ func (s *PublishedFileMissingSuite) TestConcurrentSnapshotPublishToSamePrefix(c
 		c.Assert(bullseyePublishCode, Equals, expectedCode, Commentf("Bullseye publish/update should succeed"))
 
 		// Verify ALL package files exist in the published pool
-		publishedStorage := s.context.GetPublishedStorage("")
+		publishedStorage, err := s.context.GetPublishedStorage("")
+		c.Assert(err, IsNil)
 		publicPath := publishedStorage.(aptly.FileSystemPublishedStorage).PublicPath()
 
 		missingFiles := []string{}

api/repos.go

@@ -60,7 +60,12 @@ func reposServeInAPIMode(c *gin.Context) {
 		storage = "filesystem:" + storage
 	}
 
-	publicPath := context.GetPublishedStorage(storage).(aptly.FileSystemPublishedStorage).PublicPath()
+	ps, err := context.GetPublishedStorage(storage)
+	if err != nil {
+		AbortWithJSONError(c, http.StatusNotFound, err)
+		return
+	}
+	publicPath := ps.(aptly.FileSystemPublishedStorage).PublicPath()
 	c.FileFromFS(pkgpath, http.Dir(publicPath))
 }
 

aptly/interfaces.go

@@ -95,8 +95,8 @@ type FileSystemPublishedStorage interface {
 
 // PublishedStorageProvider is a thing that returns PublishedStorage by name
 type PublishedStorageProvider interface {
-	// GetPublishedStorage returns PublishedStorage by name
-	GetPublishedStorage(name string) PublishedStorage
+	// GetPublishedStorage returns PublishedStorage by name, or an error if the storage is not configured
+	GetPublishedStorage(name string) (PublishedStorage, error)
 }
 
 // BarType used to differentiate between different progress bars

cmd/publish_snapshot.go

@@ -198,9 +198,11 @@ func aptlyPublishSnapshotOrRepo(cmd *commander.Command, args []string) error {
 
 	context.Progress().Printf("\n%s been successfully published.\n", message)
 
-	if localStorage, ok := context.GetPublishedStorage(storage).(aptly.FileSystemPublishedStorage); ok {
-		context.Progress().Printf("Please setup your webserver to serve directory '%s' with autoindexing.\n",
-			localStorage.PublicPath())
+	if ps, err := context.GetPublishedStorage(storage); err == nil {
+		if localStorage, ok := ps.(aptly.FileSystemPublishedStorage); ok {
+			context.Progress().Printf("Please setup your webserver to serve directory '%s' with autoindexing.\n",
+				localStorage.PublicPath())
+		}
 	}
 
 	context.Progress().Printf("Now you can add following line to apt sources:\n")

cmd/serve.go

@@ -97,7 +97,11 @@ func aptlyServe(cmd *commander.Command, args []string) error {
 		}
 	}
 
-	publicPath := context.GetPublishedStorage("").(aptly.FileSystemPublishedStorage).PublicPath()
+	ps, err := context.GetPublishedStorage("")
+	if err != nil {
+		return err
+	}
+	publicPath := ps.(aptly.FileSystemPublishedStorage).PublicPath()
 	ShutdownContext()
 
 	fmt.Printf("\nStarting web server at: %s (press Ctrl+C to quit)...\n", listen)

context/context.go

@@ -117,7 +117,12 @@ func (context *AptlyContext) config() *utils.ConfigStructure {
 			if err != nil {
 				fmt.Fprintf(os.Stderr, "Config file not found, creating default config at %s\n\n", homeLocation)
 
-				_ = utils.SaveConfigRaw(homeLocation, aptly.AptlyConf)
+				defaultConfig := aptly.AptlyConf
+				if len(defaultConfig) == 0 {
+					defaultConfig = []byte("root_dir: \"\"")
+				}
+
+				_ = utils.SaveConfigRaw(homeLocation, defaultConfig)
 				err = utils.LoadConfig(homeLocation, &utils.Config)
 				if err != nil {
 					Fatal(fmt.Errorf("error loading config file %s: %s", homeLocation, err))
@@ -408,8 +413,8 @@ func (context *AptlyContext) PackagePool() aptly.PackagePool {
 	return context.packagePool
 }
 
-// GetPublishedStorage returns instance of PublishedStorage
-func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedStorage {
+// GetPublishedStorage returns instance of PublishedStorage, or an error if the storage is not configured
+func (context *AptlyContext) GetPublishedStorage(name string) (aptly.PublishedStorage, error) {
 	context.Lock()
 	defer context.Unlock()
 
@@ -420,14 +425,14 @@ func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedSto
 		} else if strings.HasPrefix(name, "filesystem:") {
 			params, ok := context.config().FileSystemPublishRoots[name[11:]]
 			if !ok {
-				Fatal(fmt.Errorf("published local storage %v not configured", name[11:]))
+				return nil, fmt.Errorf("published local storage %v not configured", name[11:])
 			}
 
 			publishedStorage = files.NewPublishedStorage(params.RootDir, params.LinkMethod, params.VerifyMethod)
 		} else if strings.HasPrefix(name, "s3:") {
 			params, ok := context.config().S3PublishRoots[name[3:]]
 			if !ok {
-				Fatal(fmt.Errorf("published S3 storage %v not configured", name[3:]))
+				return nil, fmt.Errorf("published S3 storage %v not configured", name[3:])
 			}
 
 			var err error
@@ -437,12 +442,12 @@ func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedSto
 				params.EncryptionMethod, params.PlusWorkaround, params.DisableMultiDel,
 				params.ForceSigV2, params.ForceVirtualHostedStyle, params.Debug)
 			if err != nil {
-				Fatal(err)
+				return nil, err
 			}
 		} else if strings.HasPrefix(name, "gcs:") {
 			params, ok := context.config().GCSPublishRoots[name[4:]]
 			if !ok {
-				Fatal(fmt.Errorf("published GCS storage %v not configured", name[4:]))
+				return nil, fmt.Errorf("published GCS storage %v not configured", name[4:])
 			}
 
 			var err error
@@ -451,51 +456,51 @@ func (context *AptlyContext) GetPublishedStorage(name string) aptly.PublishedSto
 				params.Project, params.Endpoint, params.ACL, params.StorageClass, params.EncryptionKey,
 				params.DisableMultiDel, params.Debug)
 			if err != nil {
-				Fatal(err)
+				return nil, err
 			}
 		} else if strings.HasPrefix(name, "swift:") {
 			params, ok := context.config().SwiftPublishRoots[name[6:]]
 			if !ok {
-				Fatal(fmt.Errorf("published Swift storage %v not configured", name[6:]))
+				return nil, fmt.Errorf("published Swift storage %v not configured", name[6:])
 			}
 
 			var err error
 			publishedStorage, err = swift.NewPublishedStorage(params.UserName, params.Password,
 				params.AuthURL, params.Tenant, params.TenantID, params.Domain, params.DomainID, params.TenantDomain, params.TenantDomainID, params.Container, params.Prefix)
 			if err != nil {
-				Fatal(err)
+				return nil, err
 			}
 		} else if strings.HasPrefix(name, "azure:") {
 			params, ok := context.config().AzurePublishRoots[name[6:]]
 			if !ok {
-				Fatal(fmt.Errorf("published Azure storage %v not configured", name[6:]))
+				return nil, fmt.Errorf("published Azure storage %v not configured", name[6:])
 			}
 
 			var err error
 			publishedStorage, err = azure.NewPublishedStorage(
 				params.AccountName, params.AccountKey, params.Container, params.Prefix, params.Endpoint)
 			if err != nil {
-				Fatal(err)
+				return nil, err
 			}
 		} else if strings.HasPrefix(name, "jfrog:") {
 			params, ok := context.config().JFrogPublishRoots[name[6:]]
 			if !ok {
-				Fatal(fmt.Errorf("published JFrog storage %v not configured", name[6:]))
+				return nil, fmt.Errorf("published JFrog storage %v not configured", name[6:])
 			}
 
 			var err error
 			publishedStorage, err = jfrog.NewPublishedStorage(
 				name[6:], params)
 			if err != nil {
-				Fatal(err)
+				return nil, fmt.Errorf("error creating jfrog manager: %w", err)
 			}
 		} else {
-			Fatal(fmt.Errorf("unknown published storage format: %v", name))
+			return nil, fmt.Errorf("unknown published storage format: %v", name)
 		}
 		context.publishedStorages[name] = publishedStorage
 	}
 
-	return publishedStorage
+	return publishedStorage, nil
 }
 
 // UploadPath builds path to upload storage

context/context_test.go

@@ -2,7 +2,6 @@ package context
 
 import (
 	"fmt"
-	"os"
 	"reflect"
 	"testing"
 
@@ -81,12 +80,11 @@ func (s *AptlyContextSuite) SetUpTest(c *C) {
 
 func (s *AptlyContextSuite) TestGetPublishedStorageBadFS(c *C) {
 	// https://github.com/aptly-dev/aptly/issues/711
-	// This will fail on account of us not having a config, so the
-	// storage never exists.
-	c.Assert(func() { s.context.GetPublishedStorage("filesystem:fuji") },
-		FatalErrorPanicMatches,
-		&FatalError{ReturnCode: 1, Message: fmt.Sprintf("error loading config file %s/.aptly.conf: invalid yaml (EOF) or json (EOF)",
-			os.Getenv("HOME"))})
+	// https://github.com/aptly-dev/aptly/issues/1477
+	// GetPublishedStorage must return an error (not panic) when the
+	// requested storage is not configured.
+	_, err := s.context.GetPublishedStorage("filesystem:fuji")
+	c.Assert(err, NotNil)
 }
 
 func (s *AptlyContextSuite) TestGetPublishedStorageJFrogConfigured(c *C) {
@@ -98,18 +96,20 @@ func (s *AptlyContextSuite) TestGetPublishedStorageJFrogConfigured(c *C) {
 	utils.Config.JFrogPublishRoots = map[string]utils.JFrogPublishRoot{
 		"test": {
 			Repository:  "aptly-repo",
-			Url:         "https://example.jfrog.local/artifactory",
+			URL:         "https://example.jfrog.local/artifactory",
 			AccessToken: "token",
 			Prefix:      "public",
 		},
 	}
 
-	storage := s.context.GetPublishedStorage("jfrog:test")
+	storage, err := s.context.GetPublishedStorage("jfrog:test")
+	c.Assert(err, IsNil)
 	c.Assert(storage, NotNil)
 	c.Assert(fmt.Sprintf("%v", storage), Equals, "jfrog:aptly-repo:public")
 
 	// Ensure we get the cached object on repeated lookups.
-	storageAgain := s.context.GetPublishedStorage("jfrog:test")
+	storageAgain, err := s.context.GetPublishedStorage("jfrog:test")
+	c.Assert(err, IsNil)
 	c.Assert(storageAgain, Equals, storage)
 }
 
@@ -120,9 +120,9 @@ func (s *AptlyContextSuite) TestGetPublishedStorageJFrogMissing(c *C) {
 	s.context.configLoaded = true
 	utils.Config.JFrogPublishRoots = map[string]utils.JFrogPublishRoot{}
 
-	c.Assert(func() { s.context.GetPublishedStorage("jfrog:missing") },
-		FatalErrorPanicMatches,
-		&FatalError{ReturnCode: 1, Message: "published JFrog storage missing not configured"})
+	_, err := s.context.GetPublishedStorage("jfrog:missing")
+	c.Assert(err, NotNil)
+	c.Check(err.Error(), Equals, "published JFrog storage missing not configured")
 }
 
 func (s *AptlyContextSuite) TestGetPublishedStorageJFrogInitError(c *C) {
@@ -133,19 +133,11 @@ func (s *AptlyContextSuite) TestGetPublishedStorageJFrogInitError(c *C) {
 	utils.Config.JFrogPublishRoots = map[string]utils.JFrogPublishRoot{
 		"broken": {
 			Repository: "aptly-repo",
-			Url:        "ssh://example.local/artifactory",
+			URL:        "ssh://example.local/artifactory",
 		},
 	}
 
-	defer func() {
-		obtained := recover()
-		c.Assert(obtained, NotNil)
-
-		fatalErr, ok := obtained.(*FatalError)
-		c.Assert(ok, Equals, true)
-		c.Check(fatalErr.ReturnCode, Equals, 1)
-		c.Check(fatalErr.Message, Matches, `error creating jfrog manager: .*`)
-	}()
-
-	s.context.GetPublishedStorage("jfrog:broken")
+	_, err := s.context.GetPublishedStorage("jfrog:broken")
+	c.Assert(err, NotNil)
+	c.Check(err.Error(), Matches, `error creating jfrog manager: .*`)
 }

deb/publish.go

@@ -832,9 +832,12 @@ func (p *PublishedRepo) GetSkelFiles(skelDir string, component string) (map[stri
 // Publish publishes snapshot (repository) contents, links package files, generates Packages & Release files, signs them
 func (p *PublishedRepo) Publish(packagePool aptly.PackagePool, publishedStorageProvider aptly.PublishedStorageProvider,
 	collectionFactory *CollectionFactory, signer pgp.Signer, progress aptly.Progress, forceOverwrite bool, skelDir string) error {
-	publishedStorage := publishedStorageProvider.GetPublishedStorage(p.Storage)
+	publishedStorage, err := publishedStorageProvider.GetPublishedStorage(p.Storage)
+	if err != nil {
+		return err
+	}
 
-	err := publishedStorage.MkDir(filepath.Join(p.Prefix, "pool"))
+	err = publishedStorage.MkDir(filepath.Join(p.Prefix, "pool"))
 	if err != nil {
 		return err
 	}
@@ -1258,7 +1261,10 @@ func (p *PublishedRepo) Publish(packagePool aptly.PackagePool, publishedStorageP
 // It can remove prefix fully, and part of pool (for specific component)
 func (p *PublishedRepo) RemoveFiles(publishedStorageProvider aptly.PublishedStorageProvider, removePrefix bool,
 	removePoolComponents []string, progress aptly.Progress) error {
-	publishedStorage := publishedStorageProvider.GetPublishedStorage(p.Storage)
+	publishedStorage, err := publishedStorageProvider.GetPublishedStorage(p.Storage)
+	if err != nil {
+		return err
+	}
 
 	// I. Easy: remove whole prefix (meta+packages)
 	if removePrefix {
@@ -1271,7 +1277,7 @@ func (p *PublishedRepo) RemoveFiles(publishedStorageProvider aptly.PublishedStor
 	}
 
 	// II. Medium: remove metadata, it can't be shared as prefix/distribution as unique
-	err := publishedStorage.RemoveDirs(filepath.Join(p.Prefix, "dists", p.Distribution), progress)
+	err = publishedStorage.RemoveDirs(filepath.Join(p.Prefix, "dists", p.Distribution), progress)
 	if err != nil {
 		return err
 	}
@@ -1631,7 +1637,10 @@ func (collection *PublishedRepoCollection) CleanupAfterMultiDistToggle(published
 	}
 
 	// true→false: directly remove the per-distribution pool directories.
-	publishedStorage := publishedStorageProvider.GetPublishedStorage(published.Storage)
+	publishedStorage, err := publishedStorageProvider.GetPublishedStorage(published.Storage)
+	if err != nil {
+		return err
+	}
 	for _, component := range cleanComponents {
 		poolDir := filepath.Join(published.Prefix, "pool", published.Distribution, component)
 		if err := publishedStorage.RemoveDirs(poolDir, progress); err != nil {
@@ -1657,7 +1666,10 @@ func (collection *PublishedRepoCollection) CleanupPrefixComponentFiles(published
 	distribution := published.Distribution
 
 	rootPath := filepath.Join(prefix, "dists", distribution)
-	publishedStorage := publishedStorageProvider.GetPublishedStorage(published.Storage)
+	publishedStorage, err := publishedStorageProvider.GetPublishedStorage(published.Storage)
+	if err != nil {
+		return err
+	}
 
 	sort.Strings(cleanComponents)
 	publishedComponents := published.Components()

deb/publish_test.go

@@ -62,12 +62,12 @@ type FakeStorageProvider struct {
 	storages map[string]aptly.PublishedStorage
 }
 
-func (p *FakeStorageProvider) GetPublishedStorage(name string) aptly.PublishedStorage {
+func (p *FakeStorageProvider) GetPublishedStorage(name string) (aptly.PublishedStorage, error) {
 	storage, ok := p.storages[name]
 	if !ok {
-		panic(fmt.Sprintf("unknown storage: %#v", name))
+		return nil, fmt.Errorf("unknown storage: %#v", name)
 	}
-	return storage
+	return storage, nil
 }
 
 type PublishedRepoSuite struct {