-
Notifications
You must be signed in to change notification settings - Fork 6
feat: JSON schemas #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2d2db65 to
180546f
Compare
3439f06 to
e25bd6f
Compare
| return &Split{ | ||
| migrationVersion: migrationVersion, | ||
| name: &serializable.Name, | ||
| owner: &serializable.Owner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual testing revealed that the testtrack schema generate command is broken on main. This is the fix.
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x100356c24]
| // Deref symlink | ||
| fi, err := os.Lstat(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if fi.Mode()&os.ModeSymlink != 0 { | ||
| path, err = os.Readlink(path) | ||
| if err != nil { | ||
| continue // It's OK if this symlink isn't traversable (e.g. app was uninstalled), we'll just skip it. | ||
| } | ||
| } | ||
| // Read file | ||
| schemaBytes, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue // It's OK if this file doesn't exist (e.g. broken symlink, app was uninstalled), we'll just skip it. | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is trying to detect a broken symlink, and it isn't working correctly on main.
$ ln -s broken ~/.testtrack/schemas/broken.yml
$ testtrack create feature_gate foo_enabled --owner web_platform
$ testtrack create feature_completion retail.foo_enabled --app_version 0.0.0
Error: open broken: no such file or directory
The reason it wasn't working is because os.Readlink succeeds, even for broken symlinks, which causes os.ReadFile to be called, which fails.
I removed the symlink detection stuff. I don't think we really care if these are symlinks or not, and os.ReadFile is capable of reading a symlink. If the os.ReadFile suggests that the file doesn't exist, we'll skip it.
samandmoore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domainlgtm platformlgtm
i like the MapSlice clean-up. as we discussed, it seems like we don't need that and we should end up with the same degree of sorting at schema writing time.
In Betterment/test_track_js_client#99, I'm adding strict typechecking for splits, variants, and identifier types based on the Test Track schema format.
The schema is currently YAML, which TypeScript doesn't support natively. There are other potential solutions to this problem, but I think the simplest solution is to convert the schema file to JSON. It's a generated file, so the readability of YAML and the ability to add comments isn't as valuable.
Behavior
The CLI will attempt to use
schema.json, but fallback toschema.yml. Projects can switch to JSON with this one-liner:Linked schemas in
~/testtrack/schemascan be JSON or YAML.Implementation
We use the YAML parser to read both JSON and YAML files, which simplifies things, since all valid JSON is valid YAML.
The use of
yaml.MapSlicein serializer structs complicated the implementation. Our YAML encoder and the built-in JSON encoder sort map keys, so loading/dumping the schema still produces deterministic output. This change actually simplifies the implementation, since we just deserialize intomap[string]int.