chore: comments, cleanup, error fixes

coder · Emyrk · Sep 9, 2024 · Aug 28, 2024 · Sep 3, 2024 · Sep 3, 2024
commit 13ea99a4b46238fd73dad53a501d02c365881904
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -22,7 +22,6 @@ import (
 	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd/notifications/types"
-	"github.com/coder/coder/v2/coderd/runtimeconfig"
 
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
@@ -3522,7 +3521,7 @@ func (q *FakeQuerier) GetRuntimeConfig(_ context.Context, key string) (string, e
 
 	val, ok := q.runtimeConfig[key]
 	if !ok {
-		return "", runtimeconfig.EntryNotFound
+		return "", sql.ErrNoRows
 	}
 
 	return val, nil

diff --git a/coderd/runtimeconfig/deploymententry.go b/coderd/runtimeconfig/deploymententry.go
@@ -21,6 +21,7 @@ type SerpentEntry interface {
 
 // DeploymentEntry extends a runtime entry with a startup value.
 // This allows for a single entry to source its value from startup or runtime.
+// DeploymentEntry will never return ErrEntryNotFound, as it will always return a value.
 type DeploymentEntry[T SerpentEntry] struct {
 	RuntimeEntry[T]
 	startupValue T
@@ -52,7 +53,7 @@ func (e *DeploymentEntry[T]) Coalesce(ctx context.Context, r Resolver) (T, error
 
 	resolved, err := e.Resolve(ctx, r)
 	if err != nil {
-		if errors.Is(err, EntryNotFound) {
+		if errors.Is(err, ErrEntryNotFound) {
 			return e.StartupValue(), nil
 		}
 		return zero, err

diff --git a/coderd/runtimeconfig/deploymententry_test.go b/coderd/runtimeconfig/deploymententry_test.go
@@ -0,0 +1,132 @@
+package runtimeconfig_test
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/v2/coderd/database/dbmem"
+	"github.com/coder/coder/v2/coderd/database/dbtestutil"
+	"github.com/coder/coder/v2/coderd/runtimeconfig"
+	"github.com/coder/coder/v2/coderd/util/ptr"
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/testutil"
+	"github.com/coder/serpent"
+)
+
+func ExampleDeploymentValues() {
+	ctx := context.Background()
+	db := dbmem.New()
+	st := runtimeconfig.NewStoreManager()
+
+	// Define the field, this will usually live on Deployment Values.
+	var stringField runtimeconfig.DeploymentEntry[*serpent.String]
+	// All fields need to be initialized with their "key". This will be used
+	// to uniquely identify the field in the store.
+	stringField.Initialize("string-field")
+
+	// The startup value configured by the deployment env vars
+	// This acts as a default value if no runtime value is set.
+	// Can be used to support migrating a value from startup to runtime.
+	_ = stringField.SetStartupValue("default")
+
+	// Runtime values take priority over startup values.
+	_ = stringField.SetRuntimeValue(ctx, st.Resolver(db), serpent.StringOf(ptr.Ref("hello world")))
+
+	// Resolve the value of the field.
+	val, err := stringField.Resolve(ctx, st.Resolver(db))
+	if err != nil {
+		panic(err)
+	}
+	fmt.Println(val)
+	// Output: hello world
+}
+
+// TestSerpentDeploymentEntry uses the package as the serpent options will use it.
+// Some of the usage might feel awkward, since the serpent package values come from
+// the serpent parsing (strings), not manual assignment.
+func TestSerpentDeploymentEntry(t *testing.T) {
+	t.Parallel()
+
+	ctx := testutil.Context(t, testutil.WaitMedium)
+	db, _ := dbtestutil.NewDB(t)
+	st := runtimeconfig.NewStoreManager()
+
+	// TestEntries is how entries are defined in deployment values.
+	type TestEntries struct {
+		String runtimeconfig.DeploymentEntry[*serpent.String]
+		Bool   runtimeconfig.DeploymentEntry[*serpent.Bool]
+		// codersdk.Feature is arbitrary, just using an actual struct to test.
+		Struct runtimeconfig.DeploymentEntry[*serpent.Struct[codersdk.Feature]]
+	}
+
+	var entries TestEntries
+	// Init fields
+	entries.String.Initialize("string-field")
+	entries.Bool.Initialize("bool-field")
+	entries.Struct.Initialize("struct-field")
+
+	// When using Coalesce, the default value is the empty value
+	stringVal, err := entries.String.Coalesce(ctx, st.Resolver(db))
+	require.NoError(t, err)
+	require.Equal(t, "", stringVal.String())
+
+	// Set some defaults for some
+	_ = entries.String.SetStartupValue("default")
+	_ = entries.Struct.SetStartupValue((&serpent.Struct[codersdk.Feature]{
+		Value: codersdk.Feature{
+			Entitlement: codersdk.EntitlementEntitled,
+			Enabled:     false,
+			Limit:       ptr.Ref(int64(100)),
+			Actual:      nil,
+		},
+	}).String())
+
+	// Retrieve startup values
+	stringVal, err = entries.String.Coalesce(ctx, st.Resolver(db))
+	require.NoError(t, err)
+	require.Equal(t, "default", stringVal.String())
+
+	structVal, err := entries.Struct.Coalesce(ctx, st.Resolver(db))
+	require.NoError(t, err)
+	require.Equal(t, structVal.Value.Entitlement, codersdk.EntitlementEntitled)
+	require.Equal(t, structVal.Value.Limit, ptr.Ref(int64(100)))
+
+	// Override some defaults
+	err = entries.String.SetRuntimeValue(ctx, st.Resolver(db), serpent.StringOf(ptr.Ref("hello world")))
+	require.NoError(t, err)
+
+	err = entries.Struct.SetRuntimeValue(ctx, st.Resolver(db), &serpent.Struct[codersdk.Feature]{
+		Value: codersdk.Feature{
+			Entitlement: codersdk.EntitlementGracePeriod,
+		},
+	})
+	require.NoError(t, err)
+
+	// Retrieve runtime values
+	stringVal, err = entries.String.Coalesce(ctx, st.Resolver(db))
+	require.NoError(t, err)
+	require.Equal(t, "hello world", stringVal.String())
+
+	structVal, err = entries.Struct.Coalesce(ctx, st.Resolver(db))
+	require.NoError(t, err)
+	require.Equal(t, structVal.Value.Entitlement, codersdk.EntitlementGracePeriod)
+
+	// Test using org scoped resolver
+	orgID := uuid.New()
+	orgResolver := st.OrganizationResolver(db, orgID)
+	// No org runtime set
+	stringVal, err = entries.String.Coalesce(ctx, orgResolver)
+	require.NoError(t, err)
+	require.Equal(t, "default", stringVal.String())
+	// Update org runtime
+	err = entries.String.SetRuntimeValue(ctx, orgResolver, serpent.StringOf(ptr.Ref("hello organizations")))
+	require.NoError(t, err)
+	// Verify org runtime
+	stringVal, err = entries.String.Coalesce(ctx, orgResolver)
+	require.NoError(t, err)
+	require.Equal(t, "hello organizations", stringVal.String())
+}
diff --git a/coderd/runtimeconfig/doc.go b/coderd/runtimeconfig/doc.go
@@ -0,0 +1,10 @@
+// Package runtimeconfig contains logic for managing runtime configuration values
+// stored in the database. Each coderd should have a Manager singleton instance
+// that can create a Resolver for runtime configuration CRUD.
+//
+// TODO: Implement a caching layer for the Resolver so that we don't hit the
+// database on every request. Configuration values are not expected to change
+// frequently, so we should use pubsub to notify for updates.
+// When implemented, the runtimeconfig will essentially be an in memory lookup
+// with a database for persistence.
+package runtimeconfig
diff --git a/coderd/runtimeconfig/entry.go b/coderd/runtimeconfig/entry.go
@@ -7,8 +7,9 @@ import (
 	"golang.org/x/xerrors"
 )
 
-var ErrNameNotSet = xerrors.New("name is not set")
-
+// EntryMarshaller requires all entries to marshal to and from a string.
+// The final store value is a database `text` column.
+// This also is compatible with serpent values.
 type EntryMarshaller interface {
 	fmt.Stringer
 }

diff --git a/coderd/runtimeconfig/entry_test.go b/coderd/runtimeconfig/entry_test.go
@@ -79,16 +79,16 @@ func TestEntry(t *testing.T) {
 		// Validate that it returns that value.
 		require.Equal(t, base.String(), field.String())
 		// Validate that there is no org-level override right now.
-		_, err := field.Resolve(ctx, mgr.DeploymentResolver(db))
-		require.ErrorIs(t, err, runtimeconfig.EntryNotFound)
+		_, err := field.Resolve(ctx, mgr.Resolver(db))
+		require.ErrorIs(t, err, runtimeconfig.ErrEntryNotFound)
 		// Coalesce returns the deployment-wide value.
-		val, err := field.Coalesce(ctx, mgr.DeploymentResolver(db))
+		val, err := field.Coalesce(ctx, mgr.Resolver(db))
 		require.NoError(t, err)
 		require.Equal(t, base.String(), val.String())
 		// Set an org-level override.
-		require.NoError(t, field.SetRuntimeValue(ctx, mgr.DeploymentResolver(db), &override))
+		require.NoError(t, field.SetRuntimeValue(ctx, mgr.Resolver(db), &override))
 		// Coalesce now returns the org-level value.
-		val, err = field.Coalesce(ctx, mgr.DeploymentResolver(db))
+		val, err = field.Coalesce(ctx, mgr.Resolver(db))
 		require.NoError(t, err)
 		require.Equal(t, override.String(), val.String())
 	})
@@ -119,16 +119,16 @@ func TestEntry(t *testing.T) {
 		// Check that default has been set.
 		require.Equal(t, base.String(), field.StartupValue().String())
 		// Validate that there is no org-level override right now.
-		_, err := field.Resolve(ctx, mgr.DeploymentResolver(db))
-		require.ErrorIs(t, err, runtimeconfig.EntryNotFound)
+		_, err := field.Resolve(ctx, mgr.Resolver(db))
+		require.ErrorIs(t, err, runtimeconfig.ErrEntryNotFound)
 		// Coalesce returns the deployment-wide value.
-		val, err := field.Coalesce(ctx, mgr.DeploymentResolver(db))
+		val, err := field.Coalesce(ctx, mgr.Resolver(db))
 		require.NoError(t, err)
 		require.Equal(t, base.Value, val.Value)
 		// Set an org-level override.
-		require.NoError(t, field.SetRuntimeValue(ctx, mgr.DeploymentResolver(db), &override))
+		require.NoError(t, field.SetRuntimeValue(ctx, mgr.Resolver(db), &override))
 		// Coalesce now returns the org-level value.
-		structVal, err := field.Resolve(ctx, mgr.DeploymentResolver(db))
+		structVal, err := field.Resolve(ctx, mgr.Resolver(db))
 		require.NoError(t, err)
 		require.Equal(t, override.Value, structVal.Value)
 	})

diff --git a/coderd/runtimeconfig/manager.go b/coderd/runtimeconfig/manager.go
@@ -1,51 +1,28 @@
 package runtimeconfig
 
 import (
-	"time"
-
 	"github.com/google/uuid"
-
-	"github.com/coder/coder/v2/coderd/util/syncmap"
 )
 
-// StoreManager is the shared singleton that produces resolvers for runtime configuration.
+// StoreManager is the singleton that produces resolvers for runtime configuration.
+// TODO: Implement caching layer.
 type StoreManager struct{}
 
 func NewStoreManager() Manager {
 	return &StoreManager{}
 }
 
-func (*StoreManager) DeploymentResolver(db Store) Resolver {
+// Resolver is the deployment wide namespace for runtime configuration.
+// If you are trying to namespace a configuration, orgs for example, use
+// OrganizationResolver.
+func (*StoreManager) Resolver(db Store) Resolver {
 	return NewStoreResolver(db)
 }
 
+// OrganizationResolver will namespace all runtime configuration to the provided
+// organization ID. Configuration values stored with a given organization ID require
+// that the organization ID be provided to retrieve the value.
+// No values set here will ever be returned by the call to 'Resolver()'.
 func (*StoreManager) OrganizationResolver(db Store, orgID uuid.UUID) Resolver {
 	return OrganizationResolver(orgID, NewStoreResolver(db))
 }
-
-type cacheEntry struct {
-	value       string
-	lastUpdated time.Time
-}
-
-// MemoryCacheManager is an example of how a caching layer can be added to the
-// resolver from the manager.
-// TODO: Delete MemoryCacheManager and implement it properly in 'StoreManager'.
-// TODO: Handle pubsub-based cache invalidation.
-type MemoryCacheManager struct {
-	cache *syncmap.Map[string, cacheEntry]
-}
-
-func NewMemoryCacheManager() *MemoryCacheManager {
-	return &MemoryCacheManager{
-		cache: syncmap.New[string, cacheEntry](),
-	}
-}
-
-func (m *MemoryCacheManager) DeploymentResolver(db Store) Resolver {
-	return NewMemoryCachedResolver(m.cache, NewStoreResolver(db))
-}
-
-func (m *MemoryCacheManager) OrganizationResolver(db Store, orgID uuid.UUID) Resolver {
-	return OrganizationResolver(orgID, NewMemoryCachedResolver(m.cache, NewStoreResolver(db)))
-}