LANGTOOLS-2195 Start allowing configuration files #56
Conversation
## Context Jira Issue: https://datadoghq.atlassian.net/browse/LANGTOOLS-2195 We've found that sometimes we want to create layers that have many arguments. Like when we have a ton of files to map. This ends up causing an issue where the underlying system cannot support so many arguments and it ends up failing. We're changing the `ocitool create-layer` command to accept a configuration file (in JSON format) that can be used to supply arguments to the command. There is technically a way to solve this problem without using a JSON file and making `ocitool create-layer` understand the different syntaxes that Bazel uses for param files: https://bazel.build/rules/lib/builtins/Args#use_param_file. Those syntaxes are hard to deal with, at best. They're written for a very specific type of CLI, and we don't seem to have an easy way to support that. Instead, we choose JSON as our format as it's ubiquitous, supported natively by Bazel (so we can say `json.encode(…)` in a rule), and supported by the Go stdlib (so we don't have to try hard to parse it). It's not a hard requirement, but it makes implemnting support trivial.
|
Current dependencies on/for this PR: This comment was autogenerated by Freephite. |
|
Thanks for the review! |
|
I also forgot to put this in the description, so I'll add it in a minute. But… There is technically a way to get configuration file support with https://pkg.go.dev/github.com/urfave/cli/v2/altsrc. It's pretty convoluted in how it works. It also requires updating all of the flags and a bunch of other book keeping. It also requires the config file to have an entry for every single flag. But complexities aside, it simply doesn't work. In particular, we have some I was going to make a note upstream, but I saw we're on an older version, and the implementation changed in v3 to support maps directly. So, I'm not even sure if this is still a problem there. Even if we did update to see if the v3 version fixed this issue, |
joneshf-dd
deleted the
hardy.jones/LANGTOOLS-2195/start-allowing-configuration-files
branch
Context
Jira Issue: https://datadoghq.atlassian.net/browse/LANGTOOLS-2195
We've found that sometimes we want to create layers that have many
arguments. Like when we have a ton of files to map. This ends up causing
an issue where the underlying system cannot support so many arguments
and it ends up failing.
We're changing the
ocitool create-layercommand to accept aconfiguration file (in JSON format) that can be used to supply arguments
to the command.
There is technically a way to solve this problem without using a JSON
file and making
ocitool create-layerunderstand the different syntaxesthat Bazel uses for param files:
https://bazel.build/rules/lib/builtins/Args#use_param_file. Those
syntaxes are hard to deal with, at best. They're written for a very
specific type of CLI, and we don't seem to have an easy way to support
that.
Instead, we choose JSON as our format as it's ubiquitous, supported
natively by Bazel (so we can say
json.encode(…)in a rule), andsupported by the Go stdlib (so we don't have to try hard to parse it).
It's not a hard requirement, but it makes implemnting support trivial.
Why not
github.com/urfave/cli/v2/altsrc?There is technically a way to get configuration file support with https://pkg.go.dev/github.com/urfave/cli/v2/altsrc. It's pretty convoluted in how it works. It also requires updating all of the flags and a bunch of other book keeping. It also requires the config file to have an entry for every single flag. But complexities aside, it simply doesn't work.
In particular, we have some
cli.Genericflags. This check forcli.Genericflags will likely never succeed, because (as far as I know) there's no way to take some primitive value parsed from JSON masquerading as aninterface{}, and find itscli.Genericmethods. Even if the compiler could do that, it would almost surely be guessing which type to magic-up because once you have more than one implementation forcli.Generic, all bets are off without more type information.I was going to make a note upstream, but I saw we're on an older version, and the implementation changed in v3 to support maps directly. So, I'm not even sure if this is still a problem there. Even if we did update to see if the v3 version fixed this issue,
urfave/clidoesn't feel like the CLI package we should be using anyway. Its lack of type information means we have a bunch of bugs in our code now. We should use a different CLI package, but that's a different conversation.