fix: validate that configs/secrets used match those provided #1423
Conversation
| @@ -82,10 +83,11 @@ func WithListener(listener Listener) Option { | |||
| // pull in missing schemas. | |||
| // | |||
| // "dirs" are directories to scan for local modules. | |||
| func New(ctx context.Context, client ftlv1connect.ControllerServiceClient, moduleDirs []string, externalDirs []string, options ...Option) (*Engine, error) { | |||
| func New(ctx context.Context, client ftlv1connect.ControllerServiceClient, projConfig *projectconfig.Config, moduleDirs []string, externalDirs []string, options ...Option) (*Engine, error) { | |||
There was a problem hiding this comment.
projectconfig.Config is not the appropriate type for this use. It is the raw configuration struct used to unmarshal TOML files, so it shouldn't be used outside that scope. ModuleContext OTOH is the appropriate type to use here, but IIRC there's no mechanism to get one prior to build. This needs some more thought, but my gut instinct is telling me we need an abstraction to retrieve ModuleConfigs from a projectconfig.Config.
There was a problem hiding this comment.
I think if implementing the above is too much work we should abandon this ticket for now as it's not super high priority. Perhaps timebox it.
There was a problem hiding this comment.
Ahhhhh got it, that explains a lot. I'll table this for now and pick up one of the next issues. We've talked a few times lately about possibly refactoring the interfaces for the TOML files, so it probably makes the most sense to come up with a holistic design for that that includes this use case.
There was a problem hiding this comment.
Just happened to see #919 in next :) Just to make sure I'm reading correctly, is that issue basically the blocker to this one?
| errs := []error{} | ||
| logger := log.FromContext(ctx) | ||
|
|
||
| configsProvidedGlobally := make(map[string]bool) |
There was a problem hiding this comment.
There's quite a lot of duplication here. I think this could be refactored to use a function that takes a map of merged global+module entries and compares them to a map of used module entries, and returns errors with the differences. Pass in the "type" of entries (config or secret) as a string for more useful errors.
There was a problem hiding this comment.
Keys in the merged map could be in the form [<module>.]<key>
| } | ||
| for secretName := range secretsUsed { | ||
| if _, isProvided := secretsProvided[secretName]; !isProvided { | ||
| errs = append(errs, fmt.Errorf("secret %q is not provided in ftl-project.toml, but is required by module %q", secretName, moduleName)) |
There was a problem hiding this comment.
These errors should include the positional information from the schema.
| // Index all provided configs into configsProvided and warn for unused configs | ||
| configsProvided := maps.Clone(globalConfig) | ||
| secretsProvided := maps.Clone(globalSecrets) | ||
| if e.projectConfig != nil { |
There was a problem hiding this comment.
Don't use pointers to convey optionality.
|
Intentionally leaving this branch alive for context, but closing the PR per #1423 (comment) |
Partially fixes #1173
The 3 commands
dev,build, anddeploy(which call instantiate a new buildengine) should all log warnings (for unnecessary configs/secrets) and errors (for configs/secrets required but not provided).Future PR to close the issue:
ftl config/secret unsetshould fail if the value being removed is still referenced in a module, unless--forceor-fis present, in which case we should warn instead of erroring.