Repository navigation
Replies: 4 comments
Revision: files through
|
Review: plain Go values, sourced by the AppThis review reads the proposal against a change of direction:
Read this way, The revision below applies it. What this resolves
Several sections also go away:
Gaps the rewrite has to fill
Consequences for the other proposalsThis proposal sets "the convention every package proposal follows", so it reverses decisions in at least two sibling proposals.
Costs to state openly
|
Revision: plain Go values, no environment variablesThis revision applies the direction in the review above. humus packages are configured only with Go values. They read no environment variables and define no variable names. The App sources every value itself with Each section below replaces the section of the same name, both in the post and in the
The open questions are rewritten. Several are answered, and the review raised new ones. SummaryEvery humus package is configured with plain Go values. A package with settings exports three things:
humus reads no environment variables, defines no variable names and reads no files for the App. Instead, the App sources every value itself, in
Proposed APIThis is a convention, not a new package. Every humus package with settings exports: // Settings configures the package. Every field is a plain value, and a
// field's zero value means zero, never "use the default".
type Settings struct{ /* ... */ }
// DefaultSettings returns the package's defaults.
func DefaultSettings() Settings
// Validate reports every out-of-range field in one error, naming each one.
func (s Settings) Validate() errorThe package's constructor takes Here is an example. The final fields belong to Proposal: HTTP server foundation. type Settings struct {
// Addr is the address to listen on, as host:port. Default ":8443".
Addr string
// ReadTimeout bounds reading a whole request. Default 5s. Zero means
// no timeout, as in net/http.
ReadTimeout time.Duration
// TLS configures TLS. Default nil, which serves plain HTTP.
TLS *tls.Config
}In d := httpserver.DefaultSettings()
env := func(name string) config.Value[string] { return config.Env(s.Process.Environment, name) }
settings, err := config.Struct(func(r *config.Resolver) httpserver.Settings {
return httpserver.Settings{
Addr: r.Get(env("ADDR").Default(d.Addr)),
ReadTimeout: r.Get(env("READ_TIMEOUT").Parse(time.ParseDuration).Default(d.ReadTimeout)),
// The file is read as part of the setting, so a missing file is a
// config error named after the variable.
TLS: r.Get(env("TLS_PEM_FILE").Parse(readFile(s.Host.FS)).Parse(serverTLS).Default(d.TLS)),
}
}).Parse(func(hs httpserver.Settings) (httpserver.Settings, error) {
return hs, hs.Validate()
}).Read(ctx)
if err != nil {
return nil, err // exit 78
}
With With An App that builds settings := httpserver.DefaultSettings()
settings.Addr = ":8080"What every settings story inheritsThese replace z5labs/bedrock#556's acceptance criteria, as #622 carried them. #556's bug was a malformed value silently replaced by its default. That bug can't happen in a package that reads no variables, and bedrock's own tests cover
One App-level example test, in the shoppingcart example rather than in each package, covers the App-side pattern. A malformed variable exits 78, and the cause names the variable. Design decisions
TestingThese tests are sketches and have not been run. func TestSettings_ValidateReportsEveryBadField(t *testing.T) {
t.Parallel()
s := httpserver.DefaultSettings()
s.Addr = "no-port"
s.ReadTimeout = -time.Second
err := s.Validate()
require.ErrorContains(t, err, "Addr")
require.ErrorContains(t, err, "ReadTimeout")
}
func TestNew_RejectsInvalidSettings(t *testing.T) {
t.Parallel()
s := httpserver.DefaultSettings()
s.ReadTimeout = -time.Second
_, err := httpserver.New(s /* , ... */)
require.ErrorContains(t, err, "ReadTimeout")
}The shoppingcart example's App-level test sets Migration from today
The rest of the migration:
Consequences for #630 and #632
Open questionsAnswered by this revision:
Still open:
|
Revision: check functions per field, and the lifecycle reads its own variablesThis revision settles the open questions of the plain Go values revision. That revision's direction stands. humus packages take plain Go values, and the end user decides how they are populated. In this revision, "the end user's Each section below replaces the section of the same name in that revision. Its Migration from today still applies, except that every row of its table chains the package's check after Decisions on the open questions
SummaryEvery humus package is configured with plain Go values. A package with settings exports:
humus packages read no environment variables, define no variable names, read no files for the end user and don't import
The one place humus reads variables is the lifecycle. Proposed APIThis is a convention, not a new package. Every humus package with settings exports: // Settings configures the package. Every field is a plain value, and a
// field's zero value means zero, never "use the default".
type Settings struct{ /* ... */ }
// DefaultSettings returns the package's defaults.
func DefaultSettings() Settings
// Check<Field> reports why v is not a valid <Field>, or nil if it is.
// There is one for each field with a valid range.
func Check<Field>(v T) errorA check's error says what is wrong, such as "must not be negative", and does not name the field. Whatever reads the value adds the name. The constructor takes Here is an example. The final fields belong to Proposal: HTTP server foundation. type Settings struct {
// Addr is the address to listen on, as host:port. Default ":8443".
// Checked by CheckAddr.
Addr string
// ReadTimeout bounds reading a whole request. Default 5s. Zero means
// no timeout, as in net/http. Must not be negative; checked by
// CheckReadTimeout.
ReadTimeout time.Duration
// TLS configures TLS. Default nil, which serves plain HTTP.
TLS *tls.Config
}
// CheckReadTimeout reports why d is not a valid ReadTimeout, or nil if it
// is.
func CheckReadTimeout(d time.Duration) error {
if d < 0 {
return errors.New("must not be negative")
}
return nil
}In their d := httpserver.DefaultSettings()
env := func(name string) config.Value[string] { return config.Env(s.Process.Environment, name) }
settings, err := config.Struct(func(r *config.Resolver) httpserver.Settings {
return httpserver.Settings{
Addr: r.Get(env("ADDR").Default(d.Addr).Validate(httpserver.CheckAddr)),
ReadTimeout: r.Get(env("READ_TIMEOUT").Parse(time.ParseDuration).Default(d.ReadTimeout).Validate(httpserver.CheckReadTimeout)),
// The file is read as part of the setting, so a missing file is a
// config error named after the variable.
TLS: r.Get(env("TLS_PEM_FILE").Parse(readFile(s.Host.FS)).Parse(serverTLS).Default(d.TLS)),
}
}).Read(ctx)
if err != nil {
return nil, err // exit 78
}
With Because the check comes after
An end user who builds settings := httpserver.DefaultSettings()
settings.Addr = ":8080"The lifecycle's settings
type App struct {
Host Host
// ShutdownGracePeriod is the grace period when
// HUMUS_SHUTDOWN_GRACE_PERIOD is not set. The grace period must be
// positive, so a zero ShutdownGracePeriod fails startup unless the
// variable is set.
ShutdownGracePeriod time.Duration
// DrainDelay is the drain delay when HUMUS_SHUTDOWN_DRAIN_DELAY is not
// set. The drain delay must not be negative.
DrainDelay time.Duration
Init func(ctx context.Context, s *Setup) ([]Service, error)
}Step 1 of config.Env(p.Environment, "HUMUS_SHUTDOWN_GRACE_PERIOD").
Parse(time.ParseDuration).
Default(a.ShutdownGracePeriod).
Validate(positive)The lifecycle's checks stay unexported, because nobody else sources these two values. They are
This is the only exception to "humus defines no variable names". It is stated here so that it isn't copied: no humus package reads a variable. What every settings story inheritsThese replace the previous revision's list.
One example test covers the end user's side: a malformed variable and an out-of-range variable each exit 78, and the cause names the variable. It lives with the first runnable example, not in each package. Design decisions
TestingThese tests are sketches and have not been run. func TestCheckReadTimeout(t *testing.T) {
t.Parallel()
require.NoError(t, httpserver.CheckReadTimeout(5*time.Second))
require.EqualError(t, httpserver.CheckReadTimeout(-time.Second), "must not be negative")
}
func TestNew_RejectsEveryBadField(t *testing.T) {
t.Parallel()
s := httpserver.DefaultSettings()
s.Addr = "no-port"
s.ReadTimeout = -time.Second
_, err := httpserver.New(s /* , ... */)
require.ErrorContains(t, err, "Addr")
require.ErrorContains(t, err, "ReadTimeout")
}
func TestNew_AcceptsDefaultSettings(t *testing.T) {
t.Parallel()
_, err := httpserver.New(httpserver.DefaultSettings() /* , ... */)
require.NoError(t, err)
}The lifecycle's tests run a
Consequences for #630 and #632
Open questionsNone. Each service package's proposal answers question 2 against its own fields. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Note
This post is superseded by three revisions below, read in order: files through
Host.FS, plain Go values, and check functions per field, and the lifecycle reads its own variables, which settles every open question. humus packages take plain Go values and export a check function per field, and the end user sources each value and chains its check. The Background, Out of scope and References sections still apply. The rest of this post is kept for context.Note
Part of the humus re-evaluation; see Proposal: humus on the bedrock process model. It depends on the charter, and it sets the convention every package proposal follows. It takes over the scope of #622, including the acceptance criterion that #622 carried for z5labs/bedrock#556.
Summary
Every humus setting is a plain Go value in a package's
Settingsstruct. The package'sReadSettingsreads that struct from the process environment with bedrock'sconfigpackage, and the package documentation lists each variable in a table. Code can override any setting by changing the struct.Host, so a missing file is a config error too.Background
bedrockconfig.Reader[T](rest/rest.go#L134-L197), with defaults frombedrockconfig.Default. At bedrock v0.21.0, a malformed value silently became the default throughconfig.MustOr(runtime/http/runtime.go:156), soHUMUS_REST_READ_TIMEOUT=30gave a 5s timeout.configpackage composesValue[T]s by chaining:config.ErrNotFound, andOr/Defaultrecover from it and from nothing else;config.Structreads many values and reports every failure in one error;config NAME: cause(config/doc.go).config.Value[T]. It also carried story(runtime): report malformed config instead of silently defaulting bedrock#556's acceptance criterion, which bedrock closed because the code it targeted moved to humus. story(config): migrate humus options to bedrock config values #622 is closed now, so this proposal carries that criterion.restreadsHUMUS_REST_*;queue/kafkareads no environment variables at all, so brokers, group ID and TLS are Go values the user must find somewhere;grpcandjobhave no settings.os.ReadFile(rest/tls.go#L50), and bedrock leaves the filesystem outside its boundary.default_config.yamlin each package) existed before story(issue-484): rest migrate to bedrock v0210 #485 deleted them.Proposed API
This is a convention, not a new package. Every humus package with settings exports:
Its package documentation lists each variable, its default and its meaning, as bedrock's
example/httpserverdoes.Here is an example. The final fields belong to Proposal: HTTP server foundation.
In
Init, a setting can be overridden by assigning to it. Code wins over the environment:With
HUMUS_HTTP_READ_TIMEOUT=30and a missingHUMUS_HTTP_TLS_PKCS12_FILE, the App exits 78 and writes this to stderr:Conventions
HUMUS_<AREA>_<SETTING>in upper snake case.AREAis the package (HTTP,GRPC,KAFKA,HEALTH), orSHUTDOWN/LOGfor the lifecycle.OTEL_*from the OTel spec; see Proposal: OpenTelemetry without globals.DATABASE_URL, are read inInitwithconfig.Env, and humus gives them no prefix.time.ParseDuration, with a unit required;strconv.ParseBool;config.OneOf;host:port.What every settings story inherits
These are z5labs/bedrock#556's acceptance criteria, as #622 carried them:
HUMUS_*variables to a malformed value, and asserts exit 78 with a cause that names the variable.t.Setenv. Tests usebedrock.NewStaticEnvironmentorbedrocktest.WithEnv.Design decisions
Plain
Settingsrather thanconfig.Value[T]options. ASettingsvalue:config.Valuestays the tool for reading settings, not the API surface. The trade-off against story(config): migrate humus options to bedrock config values #622's direction is real: with lazy options, a user could plug a differentSourceinto each field, such as a secret manager. With plainSettings, they build the struct themselves withconfig.Struct.Files are read through
Host.ReadFilewhile settings are read. That way a missing or unreadable file fails startup with exit 78 and the variable's name, instead of failing later inside a constructor.No config files for now. Only the environment is read. Every setting is a
config.Valueunderneath, so a file-backedSourcecan be added later without changing anySettingstype.No flags in humus packages.
ReadSettingsnever readsp.Args. An App that wants flags parses them inInitwith bedrock's idiom.No registry or validation framework. Range checks live in the parse functions, as in bedrock's
parsePort.Testing
The prototype attached to Proposal: the humus App lifecycle includes these tests, which pass:
An App-level test asserts the same through the whole App: exit code 78, the cause, and
p.Stderr()from bedrocktest.Migration from today
rest.Port(r),ReadTimeout(r)and the otherbedrockconfig.Readeroptions become fields of the HTTP server'sSettings.HUMUS_REST_PORT=8443becomesHUMUS_HTTP_ADDR=:8443. The final names are in Proposal: HTTP server foundation.HUMUS_REST_TLS_PKCS12_FILEis read throughHost.ReadFileinstead ofos.ReadFile.HUMUS_KAFKA_*variables (Proposal: queue processing and the Kafka runtime).os.Getenv("DATABASE_URL")becomesconfig.Env(s.Process.Environment, "DATABASE_URL")inInit.Open questions
Settings, orconfig.Value[T]options (story(config): migrate humus options to bedrock config values #622's direction)?HUMUS_for everything humus owns, or common unprefixed names such asPORT?ReadSettingstake a prefix?Sourceadded later?strconvecho the input (parsing "hunter2": invalid syntax). Should humus provide aSecrettype that redacts itself in errors and logs, or a rule that secrets are never parsed by a function that echoes its input?ReadSettingssignatures. TakereadFileonly where files are involved, or always take the same arguments?Out of scope
References
configpackageAll reactions