New YAML loader to support configuration location #828
Conversation
There was a problem hiding this comment.
Looks really nice. I can already see a lot of possibilities with language servers here.
The only concern I have is with speed. How does this scale with project size and when we collect more data (eg. comments)? We need to call this multiple times from vscode.
Can we maybe add a "load test" once we have the entire command (to generate a consolidated JSON view of the bundle) wired up?
Maybe this is a non issue because we never see too big or too recursive bundles. But would be nice to be sure.
libs/config/yamlloader/loader.go
Outdated
| case "!!merge": | ||
| merge = val | ||
| continue |
There was a problem hiding this comment.
I guess there is only one merge node in each mapping? Any sense in panicking if merge is non-nil and we hit this codepath?
There was a problem hiding this comment.
That is correct. It is not allowed to use the same key multiple times, which means that specifying << twice is not valid. To merge multiple anchors, the value for << can be an array. I checked and the parser returns an error if you specify the same key twice.
I'll still add the panic, though, that is cheap.
libs/config/yamlloader/loader.go
Outdated
| } | ||
| m, ok := v.AsMap() | ||
| if !ok { | ||
| return config.NilValue, merr |
There was a problem hiding this comment.
Can we indicate to the user which anchor is incorrect and what its true type is? Would help with debugging.
There was a problem hiding this comment.
The error comes from merr and includes the location information where it is defined.
For example:
yaml (testdata/error_02.yml:5:7): map merge requires map or sequence of maps as the value
Where 5:7 maps to the opening bracket in the merge key:
# Use string anchor inside sequence to extend a mapping.
str: &str "Hello world!"
map:
<<: [*str]
key: value
libs/config/yamlloader/loader.go
Outdated
| seq = append(seq, m) | ||
| } | ||
| case yaml.AliasNode: | ||
| v, err := d.load(merge) |
There was a problem hiding this comment.
Do we ever intend this pathway and that inside the loop in the SequenceNode branch to diverge? If not, it may make sense to abstract this logic into a standalone function to ensure consistency over time.
There was a problem hiding this comment.
We do not. I rewrote this to first get a slice of *yaml.Node and then do the processing to abstract it.
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
CLI: * Never load authentication configuration from bundle for sync command ([#889](#889)). * Fixed requiring positional arguments for API URL parameters ([#878](#878)). Bundles: * Add support for validating CLI version when loading a jsonschema object ([#883](#883)). * Do not emit wheel wrapper error when python_wheel_wrapper setting is true ([#894](#894)). * Resolve configuration before performing verification ([#890](#890)). * Fix wheel task not working with with 13.x clusters ([#898](#898)). Internal: * Skip prompt on completion hook ([#888](#888)). * New YAML loader to support configuration location ([#828](#828)). Dependency updates: * Bump github.com/mattn/go-isatty from 0.0.19 to 0.0.20 ([#896](#896)).
## Changes Now that we have a new YAML loader (see #828), we need code to turn this into our Go structs. ## Tests New unit tests pass. Confirmed that we can replace our existing loader/converter with this one and that existing unit tests for bundle loading still pass.
Changes
In order to support variable interpolation on fields that aren't a string in the resource types, we need a separate representation of the bundle configuration tree with the type equivalent of Go's
any. But instead of usinganydirectly, we can do better and use a custom type equivalent toanythat captures additional metadata. In this PR, the additional metadata is limited to the origin of the configuration value (file, line number, and column).The YAML in this commit uses the upstream YAML parser's
yaml.Nodetype to get access to location information. It reimplements the loader that takes theyaml.Nodestructure and turns it into the configuration tree we need.Next steps after this PR:
jsonloaderthat produces the same tree and includes location informationTests
The tests in
yamlloaderperform an equality check on the untyped output of loading a YAML file between the upstream YAML loader and this loader. The YAML examples were generated by prompting ChatGPT for examples that showcase anchors, primitive values, edge cases, etc.