-
Notifications
You must be signed in to change notification settings - Fork 169
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
Jsonnet: Move the JsonnetImplentation
option upwards
#917
Conversation
Since #916, the JsonnetImplementation option is not used in the jsonnet package, so the jsonnet package doesn't need to know it This removes the option there and shift it up to the `tanka.Opts` struct which is where the jsonnet implementation to use is determined
|
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.
Cool.
What do you think about moving to a more maintained CLI library in a future PR? When I was doing the shuffling yesterday I wanted to add validation when the args were parsed but it didn't look possible with the one we are using. Then you could convert to the actual implementation right away and not have to deal with strings deeper in the code.
} | ||
|
||
func workflowFlags(fs *pflag.FlagSet) *workflowFlagVars { | ||
v := workflowFlagVars{} | ||
fs.StringVar(&v.name, "name", "", "string that only a single inline environment contains in its name") | ||
fs.StringSliceVarP(&v.targets, "target", "t", nil, "Regex filter on '<kind>/<name>'. See https://tanka.dev/output-filtering") | ||
fs.StringVar(&v.jsonnetImplementation, "jsonnet-implementation", "go", "Use `go` to use native go-jsonnet implementation and `binary:<path>` to delegate evaluation to a binary (with the same API as the regular `jsonnet` binary)") |
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.
Is binary
merged?
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.
Oh, no, but it'll definitely be before a release so 🤷
Yeah, would be nice. Though, it seems medium-high effort for low benefit to me, so it's definitely not a priority |
Since Iain's change (#914 (comment)) to my PR, the JsonnetImplementation option is not used in the
jsonnet
package, so thejsonnet
package doesn't need to know about itThis removes the option there and shift it up to the
tanka.Opts
struct which is where the jsonnet implementation to use is determined