fix: enable configuring log_dir from runtime config

[kate-goldenring]

Currently, there is a bug in the ResolvedRuntimeConfig that makes it impossible to configuring log_dir from a runtime configuration file. Doing so results in the following error:

Error: unused runtime config key(s): log_dir

This appears to be because the runtime config finalizer validates that all entries of the config are visited before the log_dir entry is referenced. Referencing it before the runtime config source is finalized fixes the issue. Making sure that the construction of TomlRuntimeConfigSource consumes the toml_resolver may prevent accidentally retroactively referencing fields.

Repro

Here is a repro with a simple TODO app that creates a SQLite DB in the state store:

$ git clone git@github.com:rylev/spin-todo.git
$ cd spin-todo
# create a directory for the state store
$ mkdir foo
# configure the state dir in runtime config
$ echo log_dir=\"$PWD/foo\" > runtime-config.toml
$ spin build -u --sqlite="@migration.sql" --runtime-config-file runtime-config.toml
Error: unused runtime config key(s): log_dir

With these changes, the logs are successfully output under the log dir at $PWD/foo as configured in runtime config

[itowlson]

I admit that even with your explanation and guidance I found it hard to follow what was going on here! That try_into looks so innocent and because traits it's not obvious how it relates to schema validation!

So... your fix looks fine. But... I think it would be super useful to add some comments on the existing code, specifically something on that try_into or the source constructor indicating that all values (or all values except what is being consumed by T? except we don't know what T is???) must have been consumed by this point, or something like that?

To reinforce this, it might be good to visually split it up into "get the values" and "get the T" stanzas - at the moment the break happens after the SQLite resolver, whereas perhaps it would guide the eye better to move the log_dir above that break, and maybe the toml? I am not sure what that does or how it relates to the making of the T.

On the other hand if you feel that's out of scope for this PR then no worries, we can add comments as a separate thing later.

The other thing I'd consider is adding a test, in the hope of avoiding a regression if some bearded fool decides he could "streamline" the code by inlining the log_dir extractor into the Self constructor. It might seem silly but if the code has these subtle ordering dependencies then I think it's worth it.

Sorry for being so niggly... the actual change is fine... if I'm making too heavy weather of it then no worries, we can add that stuff later!

[kate-goldenring]

@itowlson I added a couple tests and a comment. I agree that we could accidentally bump into this, so hopefully the tests will cover those cases. If we add more fields to the runtime config, we should add them to these tests

[itowlson]

Looks great now Kate, thanks for adding all this!