cmd/root: Add support for configuration files #828

HarryMichal · 2021-07-03T22:05:11Z

Some aspects of Toolbox could be configured (e.g., the default used
image). Let's enable that!

Some aspects of Toolbox could be configured (e.g., the default used image). Let's enable that! containers#828

softwarefactory-project-zuul · 2021-07-03T22:17:04Z

Build failed.

unit-test : FAILURE in 3m 27s
system-test-fedora-33 : FAILURE in 4m 24s
system-test-fedora-34 : FAILURE in 5m 52s
system-test-fedora-rawhide : FAILURE in 5m 16s

It was not apparent on first glance that the second parameter represented a release (resp. tag). containers#828

Some aspects of Toolbox could be configured (e.g., the default used image). containers#828

It was not apparent on first glance that the second parameter represented a release (resp. tag). containers#828

ResolveContainerAndImageNames() function does too much work. It makes more sense to have two functions: one for image name resolving and other for container name resolving. containers#828

The file format is TOML[0] but the file extension is .conf to follow the convention set by Podman. containers#828

Some aspects of Toolbox could be configured (e.g., the default used image). containers#828

containers#828

softwarefactory-project-zuul · 2021-07-08T14:08:28Z

Build failed.

unit-test : SUCCESS in 3m 57s
system-test-fedora-33 : SUCCESS in 11m 08s
system-test-fedora-34 : SUCCESS in 20m 01s
system-test-fedora-rawhide : TIMED_OUT in 20m 58s

debarshiray

Thanks for working on this, @HarryMichal ! Some quick comments:

data/config/toolbox.conf

src/cmd/root.go

src/pkg/utils/utils.go

The ResolveContainerAndImageNames() function does too much work. It makes more sense to have two functions: one for resolving the image name and another for resolving the container name. containers#828

The ResolveContainerAndImageNames() function does too much work. It makes more sense to have two functions: one for resolving the image name and another for resolving the container name. containers#828 containers#838

debarshiray · 2021-07-13T11:10:39Z

I split out the break-up of ResolveContainerAndImageNames into #838

src/cmd/root.go

src/pkg/utils/utils.go

data/config/toolbox.conf

Figuring out the default toolbox image for a given distribution only needs to happen as part of resolving the final toolbox image name from the given command line options. Fallout from c990fb4 containers#828

The file format is TOML[0] but the file extension is .conf to follow the convention set by Podman. containers#828

A subsequent commit will add support for configuration files, which can override the default toolbox image. Since this override affects all commands, it effectively ends up adding a fourth option to the 'enter' command, other than the existing options to change the distribution, release and container. This makes it a lot more difficult to reason when only 'toolbox enter --release N' is enough to enter the created container. containers#828 containers#851

The file format is TOML[0] but the file extension is .conf to follow the convention set by Podman. containers#828 containers#851

It looks like there are some oddities with Viper [1]. The errors can't be examined with errors.As [2] and Viper doesn't actually throw ConfigFileNotFoundError if a configuration file is not found. Secondly, there's no way to find out if a key was actually specified in a configuration file. The InConfig API doesn't return 'true' even if a key was mentioned in a configuration file, and the IsSet API returns 'true' even if the key was only set via SetDefault in the code. Some changes by Debarshi Ray. [1] https://pkg.go.dev/github.com/spf13/viper [2] https://blog.golang.org/go1.13-errors containers#828 containers#851

The file format is TOML[0] but the file extension is .conf to follow the convention set by Podman. containers#828 containers#851

It looks like there are some oddities with Viper [1]. The errors can't be examined with errors.As [2] and Viper doesn't actually throw ConfigFileNotFoundError if a configuration file is not found. Secondly, there's no way to find out if a key was actually specified in a configuration file. The InConfig API doesn't return 'true' even if a key was mentioned in a configuration file, and the IsSet API returns 'true' even if the key was only set via SetDefault in the code. Some changes by Debarshi Ray. [1] https://pkg.go.dev/github.com/spf13/viper [2] https://blog.golang.org/go1.13-errors containers#828 containers#851

HarryMichal · 2021-07-28T13:29:50Z

Closing. The work on tests can be done in a separate PR.

Follow up to: - containers#828 - containers#851

The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. Splitting things even further might make it worse. A better approach might be to (re-)unify the code further. This is the first step towards that. This reverts the following commits: * 5c8ad7a * 02f45fd * 8b6418d ... but retains the test cases that were not tied to the changes in those commits. [1] Commit fd75608 containers#828 containers#838 [2] containers#977 containers#937

The idea of splitting ResolveContainerAndImageNames into two public functions [1] didn't turn out to be so useful [2]. It pushed the burden on the callers, who needed to carefully call the split functions in the right order, because the container, distro, image and release values are very tightly related. This opens the door for mistakes. A better approach would be to restore ResolveContainerAndImageNames as the single public API. If necessary it could be internally split into smaller private functions. It would keep things simple for the callers. Note that this commit doesn't include the private split. If necessary, it can be done in future. This reverts commit fd75608. [1] Commit fd75608 containers#828 containers#838 [2] containers#977 containers#937