This builds on top of commit 0465d78fd9034ce9.
The toolboxImage type has been renamed to Image and moved into the
podman package.
There is nothing Toolbx specific about the type - it represents any
image returned by 'podman images'. The images are only later filtered
for Toolbx images.
Secondly, having the Image type inside the podman package makes it
possible to encapsulate the unmarshalling of the JSON within the package
without exposing the raw JSON to outside consumers. This is desirable
because the unmarshalling involves tracking changes in the JSON output
by different Podman versions, and it's better to limit such details to
the podman package.
https://github.com/containers/toolbox/pull/1190
It's better to avoid single letter variables in general, because they
are so hard to grep for.
This will make the subsequent commit easier to read.
https://github.com/containers/toolbox/pull/1190
This builds on top of commit e772207831.
Currently, the JSON from 'podman images --format json' gets unmarshalled
into a []map[string]interface{} in podman.GetImages, where the maps in
the slice represent images. Each map is then marshalled back into JSON
and then again unmarshalled into a toolboxImage type.
This is wasteful. The toolboxImage type already implements the
json.Unmarshaler interface [1], since commit e772207831. Hence,
the entire JSON from 'podman images --format json' can be directly
unmarshalled into a slice of toolboxImages without involving the
[]map[string]interface{}.
A subsequent commit will move the toolboxImage type into the podman
package to more tightly encapsulate the unmarshalling of the JSON. So,
as an intermediate step in that direction, the podman.GetImages function
has been temporarily changed to return the entire JSON.
[1] https://pkg.go.dev/encoding/json#Unmarshalerhttps://github.com/containers/toolbox/pull/1190
Commit ae43560d45 had added a test with a similar intention. When
the test suite is run on a Fedora Rawhide host, it tests whether the
containers for the two previous stable Fedora releases start or not.
Fedora N-2 reaches End of Life 4 weeks after Fedora N is released [1].
So, testing the containers for Fedora Rawhide and the two previous
stable releases on a Fedora Rawhide host is a decent test of general
backwards compatibility.
However, as seen recently [2], this isn't enough to catch some known
ABI compatibility issues [3,4]. These involve toolbox binaries built
on hosts with newer toolchains that aren't meant to be run against
containers with older runtimes. A targeted test is needed to defend
against these scenarios.
The fedora-toolbox:34 image has glibc-2.33, which is old enough to be
unable to run binaries compiled on Fedora 35 with glibc-2.34 and newer.
[1] https://docs.fedoraproject.org/en-US/releases/
[2] https://github.com/containers/toolbox/pull/1180
[3] Commit 6063eb27b9https://github.com/containers/toolbox/issues/821
[4] Commit 6ad9c63180https://github.com/containers/toolbox/issues/529https://github.com/containers/toolbox/pull/1187
Fedora 32 reached End of Life on 25th May 2021:
https://docs.fedoraproject.org/en-US/releases/eol/
That's quite old because right now Fedora 35 is nearing its End of Life.
Since the tests are intended for Toolbx, not the Fedora infrastructure,
it will be better to use a newer image, because images that are too old
can get lost from registry.fedoraproject.org. The fedora-toolbox:34
image can be a drop-in replacement for the fedora-toolbox:32 image for
the purposes of this test suite, and has the advantage of being newer.
Note that fedora-toolbox:34 is also old enough to test that the toolbox
binary runs against it's build-time ABI from the host, and not the
Toolbx container's ABI, when it's invoked as the entry point of the
container [1,2]. This is important because the subsequent commit will
add a test to ensure that.
[1] Commit 6063eb27b9https://github.com/containers/toolbox/issues/821
[2] Commit 6ad9c63180https://github.com/containers/toolbox/issues/529https://github.com/containers/toolbox/pull/1187
Otherwise, there's so much spew from 'go test', including the successful
tests, that the actual failures don't stand out.
Note that, the different steps involved in building the code base are a
lot more interdependent on each other. Hence, some extra verbosity
can help understand what caused a build failure on non-interactive build
environments. In contrast, the runtime outputs from each test case are
a lot more isolated and independent from one another. The additional
verbosity from successful tests doesn't really help understand why a
particular test failed.
https://github.com/containers/toolbox/pull/1186
Currently, only a so-called high-confidence subset of the default checks
in 'go vet' are being run by 'go test' [1]. Since 'go vet' is part of
the core Go tools, it's worth trying to use more of it. After all,
golangci-lint, which is currently being run through a GitHub Action,
is running the default 'go vet' checks as one of its linters [2].
It's good to have as much of the testing wrapped inside 'meson test', as
possible, because it's easier to run locally and on other non-GitHub CI
environments like those of downstream distributors.
[1] https://pkg.go.dev/cmd/go/internal/test
[2] https://golangci-lint.run/usage/linters/https://golangci-lint.run/usage/linters/#govethttps://github.com/containers/toolbox/pull/1186
In the past, before commit d323143c46, there was either had a
dummy 'return' statement or a self-documenting 'panic' that said that
the code should not be reached. Since neither golangci-lint nor
'go vet' likes those, a comment is the only option left.
Note that the core Go tools like 'go vet' [1], but also 'go lint' [2],
explicitly don't intend to add fine-grained configuration options,
including inline directives or pragmas, to silence specific warnings.
That's something golangci-lint offers [3], to the extent that it's
supported by its linters [4]. However, golangci-lint also uses 'go vet'
as one of those linters, so it's the same problem all over again.
Therefore, between the two extremes of leaving the code difficult to
read and using a very big hammer to disable a needlessly big chuck of
'go vet', a comment is the least worst option.
[1] https://github.com/golang/go/issues/17058https://github.com/golang/go/issues/18432
[2] https://github.com/golang/lint/issues/263
[3] https://golangci-lint.run/usage/false-positives/
[4] https://golangci-lint.run/usage/linters/
Fallout from d323143c46https://github.com/containers/toolbox/pull/1185
Using the word 'containerized' gives the false impression of heightened
security. As if it's a mechanism to run untrusted software in a
sandboxed environment without access to the user's private data (such as
$HOME), hardware peripherals (such as cameras and microphones), etc..
That's not what Toolbx is for.
Toolbx aims to offer an interactive command line environment for
development and troubleshooting the host operating system, without
having to install software on the host. That's all. It makes no
promise about security beyond what's already available in the usual
command line environment on the host that everybody is familiar with.
https://github.com/containers/toolbox/issues/1020
Mention that Toolbx is meant for system administrators to troubleshoot
the host operating system. The word 'debugging' is often used in the
context of software development, and hence most readers might not
interpret it as 'troubleshooting'.
https://github.com/containers/toolbox/pull/1182
Otherwise, every zsh instance on Fedora Kinoite and Silverblue was
running into:
/etc/profile.d/toolbox.sh:30: bad substitution
... because case modification with "${VARIANT_ID^}" is undefined in
POSIX shell [1], and doesn't work with Z shell.
Fedora Silverblue got its own PRETTY_NAME (and VARIANT and VARIANT_ID)
starting from Fedora 32 [2]. Therefore, it's better to use PRETTY_NAME
and let the downstream distributor of the host operating system decide
how it should be presented to the user, instead of coming up with a
custom string. eg., PRETTY_NAME isn't the same as "Fedora $VARIANT" on
Fedora Silverblue.
One nice side-effect of this is that while VARIANT and VARIANT_ID are
optional fields, PRETTY_NAME has a well-defined fallback value of
'Linux' [3]. This makes this a little less specific to Fedora Kinoite
and Silverblue.
The rest of the welcome text was reformatted to prevent it from getting
too wide depending on the contents of PRETTY_NAME.
Fallout from 3641a0032f
[1] https://www.shellcheck.net/wiki/SC3059
[2] https://pagure.io/workstation-ostree-config/c/c18ef957d11862d32f362722931dbfdf1f5beb0d
[3] https://www.freedesktop.org/software/systemd/man/os-release.htmlhttps://github.com/containers/toolbox/issues/1017
On a couple of occasions the relevant tests didn't get triggered because
some files weren't listed [1], and on another a commit forgot to update
the list of files [2].
The objective of the CI is to reduce stress for the maintainers, and
make it easy for contributors to find out if their changes work or not.
Missing tests don't help with that, and there's no need to optimize the
tests like this unless there's a real problem to be solved.
[1] Commit deca452b27
Commit 5c27d73021
[2] Commit b1743c4927
This reverts commit c28d902089.
https://github.com/containers/toolbox/pull/1168
Some OSTree based systems, such as Endless OS, don't ship with
/usr/lib/os-release, and the os-release(5) manual says [1]:
The file /etc/os-release takes precedence over /usr/lib/os-release.
Applications should check for the former, and exclusively use its data
if it exists, and only fall back to /usr/lib/os-release if it is
missing.
[1] https://www.freedesktop.org/software/systemd/man/os-release.htmlhttps://github.com/containers/toolbox/pull/692
This is a precursor to checking that higher valued exit codes from the
command running inside the container are retained, and commands like
test(1) can be used with 'toolbox run ...' in subsequent test cases.
https://github.com/containers/toolbox/pull/1163
Currently, some of the names of the tests were too long, and had
inconsistent and verbose wording. This made it difficult to look at
them and get a gist of all the scenarios being tested. The names are
like headings. They shouldn't be too long, should capture the primary
objective of the test and be consistent in their wording.
https://github.com/containers/toolbox/pull/1161
Currently, 'meson compile' and 'meson install' were being invoked from
pre-run playbooks. This meant that a genuine build failure from either
of those commands would be shown as a RETRY_LIMIT failure by the CI.
This was misleading. It made it look as if the failure was caused by
some transient networking problem or that the CI node was too slow due
to momentary heavy load, whereas the failure was actually due to a
problem in the Toolbx sources. A genuine problem in the sources should
be reflected as a FAILURE, not RETRY_LIMIT.
However, it's worth noting that 'meson compile' invokes 'go build',
which downloads all the Go modules required by the Toolbx sources. This
is worth retaining in the pre-run playbooks since it primarily depends
on Internet infrastructure beyond the Toolbx sources.
As a nice side-effect, the CI no longer gets mysteriously stuck like
this while the Go modules are being downloaded:
TASK [Build Toolbox]
ci-node-36 | ninja: Entering directory
`/home/zuul-worker/src/github.com/containers/toolbox/builddir'
...
ci-node-36 | [8/13] Generating doc/toolbox-rmi.1 with a custom command
ci-node-36 | [9/13] Generating doc/toolbox-run.1 with a custom command
ci-node-36 | [10/13] Generating doc/toolbox.conf.5 with a custom
command
ci-node-36 | [11/13] Generating src/toolbox with a custom command
https://github.com/containers/toolbox/pull/1158
This mirrors the --preserve-fds option of Podman.
Converting an unsigned 'uint', which is what Podman uses for its
--preserve-fds option, to a string is surprisingly annoying.
strconv.Itoa [1] takes a signed 'int', which would require a cast, and
there's no unsigned counterpart. There's strconv.FormatUint [2] which
takes an unsigned 'uint64', which is better, but would still require a
cast.
So, fmt.Sprint [3] it is, if the cast is to be avoided. It's more
expensive than the other two functions, but there's no need to worry
unless it's proven to be a performance bottle neck.
Some changes by Debarshi Ray.
[1] https://pkg.go.dev/strconv#Itoa
[2] https://pkg.go.dev/strconv#FormatUint
[3] https://pkg.go.dev/fmt#Sprinthttps://github.com/containers/toolbox/issues/1066
Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
