Merge pull request #42 from sudo-bmitch/pr-conf-defaults

Consolidate setting config default values
olareg · Jan 6, 2024 · e2c8caf · e2c8caf
2 parents 04aa630 + b8fb38d
commit e2c8caf
Show file tree

Hide file tree

Showing 8 changed files with 141 additions and 47 deletions.
diff --git a/config/config.go b/config/config.go
@@ -8,6 +8,10 @@ import (
 	"github.com/olareg/olareg/internal/slog"
 )
 
+const (
+	manifestLimitDefault = 8388608 // 8MiB
+)
+
 type Store int
 
 const (
@@ -59,6 +63,32 @@ type ConfigAPIReferrer struct {
 	Enabled *bool
 }
 
+func (c *Config) SetDefaults() {
+	t := true
+	f := false
+	if c.API.DeleteEnabled == nil {
+		c.API.DeleteEnabled = &t
+	}
+	if c.API.PushEnabled == nil {
+		c.API.PushEnabled = &t
+	}
+	if c.API.Blob.DeleteEnabled == nil {
+		c.API.Blob.DeleteEnabled = &f
+	}
+	if c.API.Referrer.Enabled == nil {
+		c.API.Referrer.Enabled = &t
+	}
+	if c.API.Manifest.Limit <= 0 {
+		c.API.Manifest.Limit = manifestLimitDefault
+	}
+	switch c.Storage.StoreType {
+	case StoreDir:
+		if c.Storage.RootDir == "" {
+			c.Storage.RootDir = "."
+		}
+	}
+}
+
 func (s Store) MarshalText() ([]byte, error) {
 	var ret string
 	switch s {

diff --git a/config/config_test.go b/config/config_test.go
@@ -0,0 +1,76 @@
+package config
+
+import "testing"
+
+func TestSetDefaults(t *testing.T) {
+	c := Config{Storage: ConfigStorage{StoreType: StoreDir}}
+	c.SetDefaults()
+	if c.API.DeleteEnabled == nil || c.API.PushEnabled == nil || c.API.Blob.DeleteEnabled == nil ||
+		c.API.Referrer.Enabled == nil {
+		t.Errorf("bool default values should not be nil")
+	}
+	if c.API.Manifest.Limit == 0 {
+		t.Errorf("manifest limit should not be zero")
+	}
+	if c.Storage.RootDir == "" {
+		t.Errorf("rootDir should not be empty for StoreDir")
+	}
+}
+
+func TestStoreMarshal(t *testing.T) {
+	tt := []struct {
+		val    Store
+		str    string
+		expErr bool
+	}{
+		{
+			val: StoreDir,
+			str: "dir",
+		},
+		{
+			val: StoreMem,
+			str: "mem",
+		},
+		{
+			val:    StoreUndef,
+			str:    "unknown",
+			expErr: true,
+		},
+	}
+	for _, tc := range tt {
+		t.Run(tc.str, func(t *testing.T) {
+			t.Run("marshal", func(t *testing.T) {
+				result, err := tc.val.MarshalText()
+				if tc.expErr {
+					if err == nil {
+						t.Errorf("marshal did not fail")
+					}
+					return
+				}
+				if err != nil {
+					t.Errorf("failed to marshal: %v", err)
+					return
+				}
+				if string(result) != tc.str {
+					t.Errorf("expected %s, received %s", tc.str, string(result))
+				}
+			})
+			t.Run("unmarshal", func(t *testing.T) {
+				var s Store
+				err := s.UnmarshalText([]byte(tc.str))
+				if tc.expErr {
+					if err == nil {
+						t.Errorf("unmarshal did not fail")
+					}
+					return
+				}
+				if err != nil {
+					t.Errorf("failed to unmarshal: %v", err)
+				}
+				if tc.val != s {
+					t.Errorf("expected %d, received %d", tc.val, s)
+				}
+			})
+		})
+	}
+}
diff --git a/internal/store/dir.go b/internal/store/dir.go
@@ -291,7 +291,7 @@ func (dr *dirRepo) indexLoad(force, locked bool) error {
 			Manifests:     []types.Descriptor{},
 			Annotations:   map[string]string{},
 		}
-		if boolDefault(dr.conf.API.Referrer.Enabled, true) {
+		if *dr.conf.API.Referrer.Enabled {
 			dr.index.Annotations[types.AnnotReferrerConvert] = "true"
 		}
 	}

diff --git a/internal/store/mem.go b/internal/store/mem.go
@@ -75,7 +75,7 @@ func (m *mem) RepoGet(repoStr string) (Repo, error) {
 		log:   m.log,
 		conf:  m.conf,
 	}
-	if boolDefault(m.conf.API.Referrer.Enabled, true) {
+	if *m.conf.API.Referrer.Enabled {
 		mr.index.Annotations[types.AnnotReferrerConvert] = "true"
 	}
 	if m.conf.Storage.RootDir != "" {

diff --git a/internal/store/store.go b/internal/store/store.go
@@ -107,7 +107,7 @@ func WithLog(log slog.Logger) Opts {
 func indexIngest(repo Repo, index *types.Index, conf config.Config, locked bool) (bool, error) {
 	mod := false
 	// error if referrer API not enabled and annotation indicates this is already converted, this repo should not writable
-	if !boolDefault(conf.API.Referrer.Enabled, true) && index.Annotations != nil && index.Annotations[types.AnnotReferrerConvert] == "true" {
+	if !*conf.API.Referrer.Enabled && index.Annotations != nil && index.Annotations[types.AnnotReferrerConvert] == "true" {
 		return mod, fmt.Errorf("index.json has referrers converted with the API disabled")
 	}
 	// ensure index has schema and media type
@@ -142,7 +142,7 @@ func indexIngest(repo Repo, index *types.Index, conf config.Config, locked bool)
 	}
 
 	// convert referrers
-	if boolDefault(conf.API.Referrer.Enabled, true) && (index.Annotations == nil || index.Annotations[types.AnnotReferrerConvert] != "true") {
+	if *conf.API.Referrer.Enabled && (index.Annotations == nil || index.Annotations[types.AnnotReferrerConvert] != "true") {
 		// for each fallback tag, validate it
 		addResp := map[string][]types.Descriptor{}
 		rmDesc := []types.Descriptor{}
@@ -351,10 +351,3 @@ func repoGetIndex(repo Repo, d types.Descriptor, locked bool) (types.Index, erro
 	}
 	return i, nil
 }
-
-func boolDefault(b *bool, def bool) bool {
-	if b != nil {
-		return *b
-	}
-	return def
-}
diff --git a/internal/store/store_test.go b/internal/store/store_test.go
@@ -55,40 +55,51 @@ func TestStore(t *testing.T) {
 	}
 	tt := []struct {
 		name         string
-		store        Store
+		conf         config.Config
 		testExisting bool
 	}{
 		{
 			name: "Mem",
-			store: NewMem(config.Config{
+			conf: config.Config{
 				Storage: config.ConfigStorage{
 					StoreType: config.StoreMem,
 				},
-			}),
+			},
 		},
 		{
 			name: "Mem with Dir",
-			store: NewMem(config.Config{
+			conf: config.Config{
 				Storage: config.ConfigStorage{
 					StoreType: config.StoreMem,
 					RootDir:   "../../testdata",
 				},
-			}),
+			},
 			testExisting: true,
 		},
 		{
 			name: "Dir",
-			store: NewDir(config.Config{
+			conf: config.Config{
 				Storage: config.ConfigStorage{
 					StoreType: config.StoreDir,
 					RootDir:   tempDir,
 				},
-			}),
+			},
 			testExisting: true,
 		},
 	}
 	for _, tc := range tt {
 		tc := tc
+		tc.conf.SetDefaults()
+		var s Store
+		switch tc.conf.Storage.StoreType {
+		case config.StoreDir:
+			s = NewDir(tc.conf)
+		case config.StoreMem:
+			s = NewMem(tc.conf)
+		default:
+			t.Errorf("unsupported store type: %d", tc.conf.Storage.StoreType)
+			return
+		}
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			t.Run("Existing", func(t *testing.T) {
@@ -97,7 +108,7 @@ func TestStore(t *testing.T) {
 				}
 				t.Parallel()
 				// query testrepo content
-				repo, err := tc.store.RepoGet(existingRepo)
+				repo, err := s.RepoGet(existingRepo)
 				if err != nil {
 					t.Errorf("failed to get repo %s: %v", existingRepo, err)
 					return
@@ -195,7 +206,7 @@ func TestStore(t *testing.T) {
 			t.Run("New", func(t *testing.T) {
 				t.Parallel()
 				// get new repo
-				repo, err := tc.store.RepoGet(newRepo)
+				repo, err := s.RepoGet(newRepo)
 				if err != nil {
 					t.Errorf("failed to get repo %s: %v", newRepo, err)
 					return

diff --git a/manifest.go b/manifest.go
@@ -49,7 +49,7 @@ func (s *Server) manifestDelete(repoStr, arg string) http.HandlerFunc {
 			return
 		}
 		// if referrers is enabled, get the manifest to check for a subject
-		if boolDefault(s.conf.API.Referrer.Enabled, true) {
+		if *s.conf.API.Referrer.Enabled {
 			// wrap in a func to allow a return from errors without breaking the actual delete
 			err = func() error {
 				rdr, err := repo.BlobGet(desc.Digest)
@@ -272,7 +272,7 @@ func (s *Server) manifestPut(repoStr, arg string) http.HandlerFunc {
 				s.log.Debug("manifest blobs missing", "repo", repoStr, "arg", arg, "mediaType", mt, "errList", eList)
 				return
 			}
-			if m.Subject != nil && m.Subject.Digest != "" && boolDefault(s.conf.API.Referrer.Enabled, true) {
+			if m.Subject != nil && m.Subject.Digest != "" && *s.conf.API.Referrer.Enabled {
 				subject = m.Subject.Digest
 				referrer = &types.Descriptor{
 					MediaType:    mt,
@@ -303,7 +303,7 @@ func (s *Server) manifestPut(repoStr, arg string) http.HandlerFunc {
 				s.log.Debug("child manifests missing", "repo", repoStr, "arg", arg, "mediaType", mt, "errList", eList)
 				return
 			}
-			if m.Subject != nil && m.Subject.Digest != "" && boolDefault(s.conf.API.Referrer.Enabled, true) {
+			if m.Subject != nil && m.Subject.Digest != "" && *s.conf.API.Referrer.Enabled {
 				subject = m.Subject.Digest
 				referrer = &types.Descriptor{
 					MediaType:    mt,

diff --git a/olareg.go b/olareg.go
@@ -17,10 +17,6 @@ import (
 	"github.com/olareg/olareg/internal/store"
 )
 
-const (
-	manifestLimitDefault = 8388608 // 8MiB
-)
-
 var (
 	pathPart = `[a-z0-9]+(?:(?:\.|_|__|-+)[a-z0-9]+)*`
 	rePath   = regexp.MustCompile(`^` + pathPart + `(?:\/` + pathPart + `)*$`)
@@ -32,19 +28,14 @@ func New(conf config.Config) *Server {
 		conf: conf,
 		log:  conf.Log,
 	}
+	s.conf.SetDefaults()
 	if s.log == nil {
 		s.log = slog.Null{}
 	}
-	if s.conf.API.Manifest.Limit <= 0 {
-		s.conf.API.Manifest.Limit = manifestLimitDefault
-	}
-	switch conf.Storage.StoreType {
+	switch s.conf.Storage.StoreType {
 	case config.StoreMem:
 		s.store = store.NewMem(s.conf, store.WithLog(s.log))
 	case config.StoreDir:
-		if s.conf.Storage.RootDir == "" {
-			s.conf.Storage.RootDir = "."
-		}
 		s.store = store.NewDir(s.conf, store.WithLog(s.log))
 	}
 	return s
@@ -107,10 +98,10 @@ func (s *Server) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 		if req.Method == http.MethodGet || req.Method == http.MethodHead {
 			s.manifestGet(matches[0], matches[1]).ServeHTTP(resp, req)
 			return
-		} else if req.Method == http.MethodPut && boolDefault(s.conf.API.PushEnabled, true) {
+		} else if req.Method == http.MethodPut && *s.conf.API.PushEnabled {
 			s.manifestPut(matches[0], matches[1]).ServeHTTP(resp, req)
 			return
-		} else if req.Method == http.MethodDelete && boolDefault(s.conf.API.DeleteEnabled, true) {
+		} else if req.Method == http.MethodDelete && *s.conf.API.DeleteEnabled {
 			s.manifestDelete(matches[0], matches[1]).ServeHTTP(resp, req)
 			return
 		} else {
@@ -122,11 +113,11 @@ func (s *Server) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 			// handle blob get
 			s.blobGet(matches[0], matches[1]).ServeHTTP(resp, req)
 			return
-		} else if req.Method == http.MethodDelete && boolDefault(s.conf.API.DeleteEnabled, true) && boolDefault(s.conf.API.Blob.DeleteEnabled, false) {
+		} else if req.Method == http.MethodDelete && *s.conf.API.DeleteEnabled && *s.conf.API.Blob.DeleteEnabled {
 			// handle blob delete
 			s.blobDelete(matches[0], matches[1]).ServeHTTP(resp, req)
 			return
-		} else if matches[1] == "uploads" && req.Method == http.MethodPost && boolDefault(s.conf.API.PushEnabled, true) {
+		} else if matches[1] == "uploads" && req.Method == http.MethodPost && *s.conf.API.PushEnabled {
 			// handle blob post
 			s.blobUploadPost(matches[0]).ServeHTTP(resp, req)
 			return
@@ -135,7 +126,7 @@ func (s *Server) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 			resp.WriteHeader(http.StatusMethodNotAllowed)
 			return
 		}
-	} else if matches, ok := matchV2(pathEl, "...", "referrers", "*"); ok && boolDefault(s.conf.API.Referrer.Enabled, true) {
+	} else if matches, ok := matchV2(pathEl, "...", "referrers", "*"); ok && *s.conf.API.Referrer.Enabled {
 		if req.Method == http.MethodGet || req.Method == http.MethodHead {
 			// handle referrer get
 			s.referrerGet(matches[0], matches[1]).ServeHTTP(resp, req)
@@ -149,7 +140,7 @@ func (s *Server) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 		// handle tag listing
 		s.tagList(matches[0]).ServeHTTP(resp, req)
 		return
-	} else if matches, ok := matchV2(pathEl, "...", "blobs", "uploads", "*"); ok && boolDefault(s.conf.API.PushEnabled, true) {
+	} else if matches, ok := matchV2(pathEl, "...", "blobs", "uploads", "*"); ok && *s.conf.API.PushEnabled {
 		// handle blob upload methods
 		if req.Method == http.MethodPatch {
 			s.blobUploadPatch(matches[0], matches[1]).ServeHTTP(resp, req)
@@ -222,10 +213,3 @@ func (s *Server) v2Ping(resp http.ResponseWriter, req *http.Request) {
 		_, _ = resp.Write([]byte("{}"))
 	}
 }
-
-func boolDefault(b *bool, def bool) bool {
-	if b != nil {
-		return *b
-	}
-	return def
-}