Conversation
|
Code coverage report:
|
| js := JobSpec{ | ||
| Name: "no-jd", | ||
| ExternalJobID: "", | ||
| SchemaVersion: 0, | ||
| Type: "", | ||
| AppConfig: *b.appCfg, | ||
| } |
There was a problem hiding this comment.
Wrap hard coded app config in a job spec since it's no longer a template.
There was a problem hiding this comment.
Pull request overview
This PR removes the generic AppConfig type parameter from the bootstrap/service-factory API, standardizing job startup around bootstrap.JobSpec and moving app-config TOML decoding into each service’s Start implementation.
Changes:
- Make
bootstrap.ServiceFactory,bootstrap.Bootstrapper, andbootstrap.Optionnon-generic, and updatebootstrap.Run/NewBootstrappersignatures accordingly. - Update runners and call sites to pass/receive
bootstrap.JobSpecinstead of a typedAppConfig. - Update token verifier startup to decode
spec.AppConfiginternally; adjust bootstrap tests to match the new spec flow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
bootstrap/bootstrap.go |
Removes generics from bootstrapper and runner; runner now TOML-decodes into JobSpec; options updated. |
bootstrap/bootstrap_test.go |
Updates test factories/runner tests to use JobSpec and TOML marshal/decode path. |
verifier/cmd/token/main.go |
Updates token verifier factory to implement new ServiceFactory signature and decode spec.AppConfig. |
verifier/cmd/servicefactory.go |
Updates verifier service-factory constructors to return non-generic bootstrap.ServiceFactory. |
verifier/cmd/committee/main.go |
Updates bootstrap option usage to new non-generic WithLogLevel. |
cmd/executor/servicefactory.go |
Updates executor service-factory constructor return type to non-generic bootstrap.ServiceFactory. |
Comments suppressed due to low confidence (1)
bootstrap/bootstrap_test.go:172
- These test comments still describe the old behavior (“runner parses spec as TOML into AppConfig” /
parseTomlStrict) butrunner.StartJobnow decodes TOML intoJobSpecand passes theJobSpecthrough toServiceFactory.Start. Please update the comments to match the new flow to avoid misleading future readers.
// runner parses spec as TOML into AppConfig, then calls fac.Start(ctx, appConfig, deps)
// use empty TOML so parseTomlStrict[any] succeeds (no undecoded fields)
require.NoError(t, r.StartJob(ctx, ""))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r *runner) StartJob(ctx context.Context, config string) error { | ||
| var spec JobSpec | ||
| _, err := toml.Decode(config, &spec) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse app config toml: %w", err) | ||
| return fmt.Errorf("bootstrap: failed to parse config: %w", err) | ||
| } | ||
|
|
||
| return r.fac.Start(ctx, appConfig, r.deps) | ||
| return r.fac.Start(ctx, spec, r.deps) |
There was a problem hiding this comment.
runner.StartJob now uses toml.Decode without strict undecoded-field checking, so typos/unknown keys in the JD job spec will be silently ignored. Consider decoding in strict mode (e.g., reuse parseTOMLStrict or check md.Undecoded() and return an error) to match the strict parsing used elsewhere in bootstrap config loading.
| bootstrap.ServiceFactory | ||
|
|
There was a problem hiding this comment.
tokenVerifierFactory embeds bootstrap.ServiceFactory as an anonymous field, but the struct already defines Start/Stop and the embedded interface value is never set. This adds confusing promoted methods (and could become a nil-forwarding footgun if the interface grows); remove the embedded bootstrap.ServiceFactory field unless it’s intentionally delegated to.
| bootstrap.ServiceFactory |
| _, err := toml.Decode(spec.AppConfig, &appConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to decode app config: %w", err) | ||
| } |
There was a problem hiding this comment.
Start decodes spec.AppConfig with toml.Decode but never checks md.Undecoded(), so unknown/typo keys in token verifier config will be silently ignored. For consistency with other services’ strict config loaders (e.g., executor/committee), decode into appConfig and fail if there are any undecoded fields, ideally including them in the error message.
| _, err := toml.Decode(spec.AppConfig, &appConfig) | |
| if err != nil { | |
| return fmt.Errorf("unable to decode app config: %w", err) | |
| } | |
| md, err := toml.Decode(spec.AppConfig, &appConfig) | |
| if err != nil { | |
| return fmt.Errorf("unable to decode app config: %w", err) | |
| } | |
| if undecoded := md.Undecoded(); len(undecoded) > 0 { | |
| return fmt.Errorf("unknown app config fields: %v", undecoded) | |
| } |
| return func(b *Bootstrapper[AppConfig]) error { | ||
| func WithTOMLAppConfig(configFilePath string) Option { | ||
| return func(b *Bootstrapper) error { | ||
| configFilePath = filepath.Clean(configFilePath) |
There was a problem hiding this comment.
WithTOMLAppConfig reads from a variable path using os.ReadFile without a //nolint:gosec justification, while other bootstrap config loads explicitly suppress G304. This is likely to fail gosec (G304) in CI; add an explicit //nolint:gosec with rationale (or otherwise constrain/validate the path) to keep linting consistent.
| configFilePath = filepath.Clean(configFilePath) | |
| configFilePath = filepath.Clean(configFilePath) | |
| //nolint:gosec // G304: configFilePath is an explicit operator-provided bootstrap config path. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
With #1007 the service factories now use
bootstrap.JobSpecunconditionally, so generics are no longer needed.