Hide the Jsonnet Go implementation behind an interface

[julienduchesne]

Tanka currently evals jsonnet using the Go native code.
However, some other implementations have come up in the past years that could be worth using (ex: https://github.com/CertainLach/jrsonnet, which is much faster)

In this PR is the first step: I create an interface where all the jsonnet eval code happens. The Go Jsonnet implementation is now hidden behind this interface.

The setting can either be passed as a global flag or as an env spec attribute to be used when exporting (spec.exportJsonnetImplementation).

[github-actions]

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-08-24 17:58 UTC

[iainlane]

Cool idea, it'll be nice to be able to try alternative implementations. 👍

I did have a couple of comments (and some inline ones)

naming nit: The term "VM" feels a bit implementation-specific even though both the current ones do use it, maybe something like "Evaluator" would be more generic?

compatibility: you are shuffling public modules around and making some things private. Since Tanka follows semver, that would require a new major version. Would it be possible to keep compatibility, defaulting to the go implementation, and have this functionality be transparent unless you want to use it?

[iainlane]

It doesn't seem like it's really necessary for this method to know the filename since it's "anonymous" - this is something that go-jsonnet uses to show you nicer errors. I checked the binary implementation from #915 and it ignores this too.

Would suggest this type:

Suggested change

	EvaluateAnonymousSnippet(filename, snippet string) (string, error)
	EvaluateAnonymousSnippet(snippet string) (string, error)

And arranging for the Go implementation to have the filename as a member on the wrapper struct.

[julienduchesne]

Filename can't be on the wrapper. The same evaluator could be used to eval multiple snippets of different files

[julienduchesne]

I'd leave it there

[iainlane]

Where in the code can that happen?

Could definitely be missing something but the only caller I can find is this one which calls evaluateSnippet, also with the path, and that always calls implementation.Get which returns a new instance, not a singleton, and then we call MakeVM on that to make a new VM.

[julienduchesne]

I think the "how it's being used currently" is not entirely relevant. A Go Jsonnet VM can be re-used if the import paths or ext code do not change. So, for our concerns, a VM/Evaluator is defined by those parameters. If we add the path to the MakeEvaluator command, then the interface gets weird

Also, I'm thinking I may add the filename to the anonymous eval error message for the binary implementation as well

[iainlane]

Also, I'm thinking I may add the filename to the anonymous eval error message for the binary implementation as well

This was a good comment that made me realise how it should be done: since the thing you care about this path for is error messages, print the filename at the call site when it makes sense to do so (i.e.the place where Evaluate() is called from).

diff --git a/pkg/tanka/evaluators.go b/pkg/tanka/evaluators.go
index 64c761cb..6f95230f 100644
--- a/pkg/tanka/evaluators.go
+++ b/pkg/tanka/evaluators.go
@@ -39,7 +39,7 @@ function(%s)
 
                raw, err = jsonnet.Evaluate(path, evalScript, opts)
                if err != nil {
-                       return "", errors.Wrap(err, "evaluating jsonnet")
+                       return "", fmt.Errorf("evaluating jsonnet for '%s': %w", path, err)
                }
                return raw, nil
        }

Or something

[julienduchesne]

The Go implementation still needs the filepath to print out its stack traces though. How would you pass it?

[iainlane]

I would move the selection of the implementation outwards a bit, and then it becomes more natural to pass the path when you select the Go one.

See 82ae5ae for something like what I mean, in case that's clearer.

[julienduchesne]

Sure, that works for me. The only thing that is weird is that vm.path is ignored when you EvaluateFile, so the path does nothing in that case and you have to specify it twice. But it's not any weirder than what I was doing

Also, I like that the loaders will use the specified implementation for all operations, including listing envs

Can you merge your PR into this one, or make yours ready to review and we can close this one?

[iainlane]

Alright, cheers. I pushed that commit here, I'll give a ✔️ for your bits, guess we can consider this a cross review heh

[julienduchesne]

compatibility: you are shuffling public modules around and making some things private. Since Tanka follows semver, that would require a new major version. Would it be possible to keep compatibility, defaulting to the go implementation, and have this functionality be transparent unless you want to use it?

We're on 0.x. There are no retro-compatibility guarantees on minors. Also, the main Tanka APIs, jsonnet.Evaluate... and the CLI, stay retro-compatible so, I'd say it's all good for this to go on a minor

[iainlane]

We're on 0.x

Good point, that makes sense.