Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
tests: abstract common dirs which differ on distributions #3307
Conversation
zyga
reviewed
May 12, 2017
Some quick feedback. Overall +1 but perhaps we can make it even simpler.
| +# Default applies for: Ubuntu, Debian | ||
| +SNAPMOUNTDIR=/snap | ||
| +LIBEXECDIR=/usr/lib | ||
| +CORELIBEXECDIR=/usr/lib |
zyga
May 12, 2017
Contributor
CORELIBEXECDIR is a constant so I'd rather not have a variable for it (more obvious what is going on)
As for others, perhaps we could use UNDER_SCORE for naming them (just personal preference)
morphis
May 12, 2017
Contributor
I would like it to have a constant in the code replace with a proper variable to express what it really is. If we would be in Go/C this would be marked as const so that it can never be changed.
zyga
May 12, 2017
Contributor
But in shell this introduces clutter and non-obviousness. It can also expand to nothing (AFAIR we don't run our tests with -u set) if there's a typo.
morphis
May 15, 2017
Contributor
@zyga We name those variables else SNAPMOUNTDIR and LIBEXECDIR too, so this is just for consistency.
| @@ -68,17 +68,18 @@ fi | ||
| # declare the "quiet" wrapper | ||
| . "$TESTSLIB/quiet.sh" | ||
| +. "$TESTSLIB/dirs.sh" |
zyga
May 12, 2017
Contributor
Could we define that at top-level so that we don't have to source it anywhere? If we place those in spread.yaml's environment section of each system it will apply everywhere.
morphis
May 12, 2017
Contributor
We could but that seems to clutter spread.yaml a lot more than needed. Actually it would be quite nice if you could tell spread with the environment: list to include necessary scripts by default. something like:
environment-initializers:
- $TESTSLIB/dirs.sh
zyga
May 12, 2017
Contributor
Agreed on the possible spread improvement but consider cluttering every task.yaml vs exactly one spread.yaml
morphis
May 12, 2017
Contributor
It wont be exactly one in spread.yaml as we will have linode, qemu and external systems. So we will have this repeated at least three times. So we really need something like environment-initializers
zyga
May 12, 2017
Contributor
It will be exactly one spread.yaml, there will be many lines but still just one place to look :)
morphis
May 15, 2017
Contributor
Give that this would not rely simplify what we have to put into spread.yaml unless we have a environment-script or environment-include element lets do the following:
- We continue to use $TESTLIB/dirs.sh everywhere in task.yaml where needed for the moment
- In parallel we start working on support for environment-script / environment-include for spread
@zyga Is this OK for you?
| snap="$(mount | grep " $core" | awk '{print $1}')" | ||
| umount --verbose "$core" | ||
| # Now unpack the core, inject the new snap-exec/snapctl into it | ||
| unsquashfs "$snap" | ||
| # clean the old snapd libexec binaries, just in case | ||
| - rm squashfs-root/usr/lib/snapd/* | ||
| + rm squashfs-root/$CORELIBEXECDIR/snapd/* |
|
There are some real failures here |
fgimenez
approved these changes
May 15, 2017
LGTM thanks! just two (ignorable) suggestions.
| +// -*- Mode: Go; indent-tabs-mode: t -*- | ||
| + | ||
| +/* | ||
| + * Copyright (C) 2016 Canonical Ltd |
| +) | ||
| + | ||
| +var opts struct { | ||
| + Path bool `short:"p" long:"path" description:"When escaping/unescaping assume the string is a path"` |
fgimenez
May 15, 2017
•
Contributor
Given that we are currently using systemd-espape with a single path argument, do we need to use go-flags for this? Maybe it would be easier just using os.Args[1] and invoke it systemd-escape "mypath"
morphis
May 15, 2017
Contributor
The path argument is here simply for one case I found in the tests where systemd-escape is called with -p. We could actually remove that and don't do any argument parsing.
mvo5
approved these changes
May 17, 2017
Looks fine, one suggestion about the systemd-escape helper.
| + | ||
| + var buffer bytes.Buffer | ||
| + for _, arg := range args[1:] { | ||
| + buffer.WriteString(systemd.EscapeUnitNamePath(arg)) |
mvo5
May 17, 2017
Collaborator
You could just fmt.Printf() here instead of going through a buffer, but easy enough to do in a followup.
mvo5
May 17, 2017
Collaborator
I think I would write it as something like:
if !opts.Path {
panic("cannot use this systemd-escape without --path")
}
for i, arg := range args[1:] {
fmt.Printf(systemd.EscapeUnitNamePath(arg))
if i < len(args) {
fmt.Printf(" ")
}
}
fmt.Printf("\n")
morphis commentedMay 12, 2017
Similar to our unit tests we need to abstract the use of our snap mount directory and the libexec directory also in our spread tests. This PR starts by introducing necessary variables which ship the default values which are used for Debian and Ubuntu. tests/lib/dirs.sh will be extended in the future to adjust those variables depending the on the SPREAD_SYSTEM the tests are executed on.