refactor(api): Loader interface #459
Conversation
Introduces an abstraction over environment loading, for clearly handling inline and static loading
|
I don't mind seeing a refactoring here, but I have quite an extensive PR that will get affected if this gets merged first. Can we review and merge mine first and then come up with this API rewrite? |
Depends on your PR 😛. One of us is going to re-do some work, which sucks, so we should consider both PR's. This is a long overdue topic, and I bet whatever you wish to add becomes simpler when the refactoring happens beforehand ;) |
|
It is not very nice to put the burden on me. |
|
@Duologic can you share your PR here? Isnt it best to review which side will require less work and go that way? |
|
That PR has been through reviews already, I figured it was the lack of time on the other side that was holding it up, but apparently a rewrite of the API was going on. |
I think I made it pretty clear I merely asked you to show me the other PR, so we can review which side requires less rebasing. You gave me zero information on your PR, so this was all I could do at the time. To not escalate this further, we will go ahead with yours first. |
sh0rez
added
the
lifecycle/blocked
sh0rez
removed
the
lifecycle/blocked
|
Rebased. @Duologic @jvrplmlmn PTAL |
| envs, err := ParseParallel(paths, opts.ParseParallelOpts) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This loads the environments in parallel, taking it away will have a performance impact. A quick local test run turned out it took ~1m40s to export Grafana's environments, while on master it takes ~40s.
There was a problem hiding this comment.
I can cover this in a PR working towards multiple inline environments.
| // TODO: Re-serializing the entire env here. This is horribly inefficient | ||
| envData, err := json.Marshal(envs[0]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| env, err := spec.Parse(envData, name) |
There was a problem hiding this comment.
We could change spec.Parse to expect a manifest.Manifest object?
There was a problem hiding this comment.
I fear that is actually not as easy as it sounds.
The whole reason we use the json re-serialization is to do a deep copy of the underlying data, which is highly tricky in Go otherwise due to pointer types, etc. The resulting v1alpha1.Environment must not be affected by future changes to the original manifest.Manifest.
I'd say we first benchmark how bad this actually is, it might be easily outweighed by an order of magnitude by the Jsonnet compiler, so this might not matter at all
| baseDir: "./testdata/cases/array/", | ||
| expected: []interface{}{ | ||
| []interface{}{ | ||
| map[string]interface{}{"testCase": "nestedArray[0][0]"}, | ||
| map[string]interface{}{"testCase": "nestedArray[0][1]"}, | ||
| }, | ||
| []interface{}{ | ||
| map[string]interface{}{"testCase": "nestedArray[1][0]"}, | ||
| map[string]interface{}{"testCase": "nestedArray[1][1]"}, | ||
| }, | ||
| }, | ||
| env: nil, | ||
| }, | ||
| { | ||
| baseDir: "./testdata/cases/object/", | ||
| expected: map[string]interface{}{ | ||
| "testCase": "object", | ||
| }, | ||
| env: nil, | ||
| }, |
There was a problem hiding this comment.
These could move to a unit test for EvalJsonnet().
| return nil, err | ||
| } | ||
|
|
||
| config, err := parseStaticSpec(root, base) |
There was a problem hiding this comment.
| config, err := parseStaticSpec(root, base) | |
| env, err := parseStaticSpec(root, base) |
I took a chance at refactoring the code in
pkg/tanka/parse.go, which was modified a lot recently. These modifications revealed problems with my original design, which led to poor readability and a tangled control flow.To address this, this PR strictly separates
inlinefromstatic(spec.json) parsing. IMO this is far more readable.It is quite likely I missed some edge-cases, @Duologic is probably able to point at these.