-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: support multiple archives with "priority" field #12
feat: support multiple archives with "priority" field #12
Conversation
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.
Thanks for the PR Rafid and sorry it took so long to review it. There are a couple of high level things missing before I am able to comment on the code directly:
- Rebase to get the latest changes and remove the outdated stuff.
- Logic of archive sorting needs to take into account negative priorities, and needs to fail if two priorities are the same (take a look at the prototype I send you a while ago: https://github.com/letFunny/chisel/blob/fips-several-archives-fetch/internal/setup/setup.go#L474-L476).
- We need to make sure archive selection is somewhat stable which I think already happens in this PR. It shouldn't happen that we select an archive at some point and then we fetch from another one, or we use the information for another one (probably setting the
archive
map inslicer
already accomplishes this one).
1c49ed7
to
a3075cd
Compare
a3075cd
to
0b00dc8
Compare
This commit adds support for fetching packages from multiple archives. It introduces a new field ``archives.<archive-name>.priority`` which takes in a signed integer and specifies the priority of a certain archive. This value is particularly useful when there are multiple archives. A package is fetched from the archive with highest priority regardless of the "default" archive, unless the slice definition file of that package specifies a particular archive using the "archive" field. Example chisel.yaml: format: v1 archives: foo: version: 22.04 components: [main, universe] priority: 20 public-keys: [..] bar: version: 22.04 components: [main] default: true priority: 10 public-keys: [..] In the above example, if a package exists in both of the archives, the package will be fetched from archive "foo" since it has higher priority. Note that, archive "bar" is the default. However, if in a particular package such as below, the "archive" field is used to specify a particular archive, that particular archive will be fetched from. package: test-package archive: bar slices: ... The above package will be fetched from archive "bar", regardless of priority. Reference: Specification RK018.
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.
I cannot find in the PR how we are handling negative priorities for archives. Can you point me to where we do it?
If it is not here, can you please revise the spec and make sure that all of the requirements are present?
If an archive has a negative priority, ignore it while choosing the best archive for a package. Unless, a package particularly asks for that particular archive with negative priority. In that case, that archive is chosen for the package.
Hiya, sorry the negative priority handling was not there before. I have added 21973dc. |
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.
Thanks Rafid, this is looking very good! The only thing that may need major changes is the testing but I do not have a clear solution yet. Can you prototype a few different options and see if any look good? We are trying to:
- Use the same table tests ideally without having to change existing ones with the burden of mentioning archives when it is not needed conceptually.
- Be able to define archives manually instead of the default test archive.
- Have a clear way of seeing which archive got selected (which I think your tests already accomplish by having different version of files). When the manifest PR is in we can also very that the correct package is included in the manifest.
One idea is to have pkgs
field which can be used by the tests that want to override the default, and another field called archives
in which you can have a map of package name to archive or something like that. That way only tests that want the other association add it. I have not tried it so I have no idea if it looks clumsy or not.
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.
Just for the shake of bookkeeping, as we discussed offline, once the changes mentioned in the comments are done, we will do one final round of reviews and raise this as a draft PR in the main repo. Once the manifest is fully merged we will use the new changes from the tests in this PR and raise the final version.
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.
Thanks Rafid, mostly nitpicks I am only worried about the validation in setup.go we discussed previously.
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.
Thanks Rafid, mostly nitpicks again. I reviewed the whole PR carefully and it is looking very good.
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.
Approved pending tests. Once all the manifest PRs are in we can rework this one and have it 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.
Looks all good to me. Just one nitpick.
This merges the changes in letFunny#12 into this branch.
This commit merges the changes of 'feat: create manifest.wall'(canonical#142) into this branch. We need the changes of this commit to include the updated testing framework and changes. Co-authored-by: Alberto Carretero <[email protected]>
Github UI does not show the correct diff after base branch changes, so here they are: git diff letfunny/chisel-db-create-manifestdiff --git a/internal/setup/setup.go b/internal/setup/setup.go
index b4a7971..c568234 100644
--- a/internal/setup/setup.go
+++ b/internal/setup/setup.go
@@ -33,6 +33,7 @@ type Archive struct {
Version string
Suites []string
Components []string
+ Priority int
PubKeys []*packet.PublicKey
}
@@ -223,6 +224,28 @@ func (r *Release) validate() error {
return err
}
+ // Check for archive priority conflicts.
+ priorities := make(map[int]*Archive)
+ for _, archive := range r.Archives {
+ if old, ok := priorities[archive.Priority]; ok {
+ if old.Name > archive.Name {
+ archive, old = old, archive
+ }
+ return fmt.Errorf("chisel.yaml: archives %q and %q have the same priority value of %v", old.Name, archive.Name, archive.Priority)
+ }
+ priorities[archive.Priority] = archive
+ }
+
+ // Check that archives pinned in packages are defined.
+ for _, pkg := range r.Packages {
+ if pkg.Archive == "" {
+ continue
+ }
+ if _, ok := r.Archives[pkg.Archive]; !ok {
+ return fmt.Errorf("%s: package refers to undefined archive %q", pkg.Path, pkg.Archive)
+ }
+ }
+
return nil
}
@@ -349,9 +372,6 @@ func readSlices(release *Release, baseDir, dirName string) error {
if err != nil {
return err
}
- if pkg.Archive == "" {
- pkg.Archive = release.DefaultArchive
- }
release.Packages[pkg.Name] = pkg
}
@@ -366,11 +386,17 @@ type yamlRelease struct {
V1PubKeys map[string]yamlPubKey `yaml:"v1-public-keys"`
}
+const (
+ MaxArchivePriority = 1000
+ MinArchivePriority = -1000
+)
+
type yamlArchive struct {
Version string `yaml:"version"`
Suites []string `yaml:"suites"`
Components []string `yaml:"components"`
Default bool `yaml:"default"`
+ Priority int `yaml:"priority"`
PubKeys []string `yaml:"public-keys"`
// V1PubKeys is used for compatibility with format "chisel-v1".
V1PubKeys []string `yaml:"v1-public-keys"`
@@ -527,11 +553,15 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
}
archiveKeys = append(archiveKeys, key)
}
+ if details.Priority > MaxArchivePriority || details.Priority < MinArchivePriority {
+ return nil, fmt.Errorf("%s: archive %q has invalid priority value %d", fileName, archiveName, details.Priority)
+ }
release.Archives[archiveName] = &Archive{
Name: archiveName,
Version: details.Version,
Suites: details.Suites,
Components: details.Components,
+ Priority: details.Priority,
PubKeys: archiveKeys,
}
}
diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go
index bec0201..74d5d22 100644
--- a/internal/setup/setup_test.go
+++ b/internal/setup/setup_test.go
@@ -85,10 +85,9 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
- Slices: map[string]*setup.Slice{},
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
+ Slices: map[string]*setup.Slice{},
},
},
},
@@ -129,9 +128,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice1": {
Package: "mypkg",
@@ -191,9 +189,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice1": {
Package: "mypkg",
@@ -452,9 +449,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice1": {
Package: "mypkg",
@@ -667,9 +663,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -707,9 +702,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -748,9 +742,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -765,7 +758,7 @@ var setupTests = []setupTest{{
},
},
}, {
- summary: "Multiple archives",
+ summary: "Multiple archives with priorities",
input: map[string]string{
"chisel.yaml": `
format: chisel-v1
@@ -775,11 +768,13 @@ var setupTests = []setupTest{{
components: [main, universe]
suites: [jammy]
default: true
+ priority: 20
v1-public-keys: [test-key]
bar:
version: 22.04
components: [universe]
suites: [jammy-updates]
+ priority: -10
v1-public-keys: [test-key]
v1-public-keys:
test-key:
@@ -788,6 +783,7 @@ var setupTests = []setupTest{{
`,
"slices/mydir/mypkg.yaml": `
package: mypkg
+ archive: foo
`,
},
release: &setup.Release{
@@ -799,6 +795,7 @@ var setupTests = []setupTest{{
Version: "22.04",
Suites: []string{"jammy"},
Components: []string{"main", "universe"},
+ Priority: 20,
PubKeys: []*packet.PublicKey{testKey.PubKey},
},
"bar": {
@@ -806,6 +803,7 @@ var setupTests = []setupTest{{
Version: "22.04",
Suites: []string{"jammy-updates"},
Components: []string{"universe"},
+ Priority: -10,
PubKeys: []*packet.PublicKey{testKey.PubKey},
},
},
@@ -818,6 +816,53 @@ var setupTests = []setupTest{{
},
},
},
+}, {
+ summary: "Two archives cannot have same priority",
+ input: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ suites: [jammy]
+ priority: 20
+ v1-public-keys: [test-key]
+ bar:
+ version: 22.04
+ components: [universe]
+ suites: [jammy-updates]
+ priority: 20
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/mypkg.yaml": `
+ package: mypkg
+ `,
+ },
+ relerror: `chisel.yaml: archives "bar" and "foo" have the same priority value of 20`,
+}, {
+ summary: "Invalid archive priority",
+ input: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ suites: [jammy]
+ priority: 10000
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ },
+ relerror: `chisel.yaml: archive "foo" has invalid priority value 10000`,
}, {
summary: "Extra fields in YAML are ignored (necessary for forward compatibility)",
input: map[string]string{
@@ -861,9 +906,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -887,12 +931,13 @@ var setupTests = []setupTest{{
components: [main, universe]
suites: [jammy]
v1-public-keys: [extra-key]
- default: true
+ priority: 20
bar:
version: 22.04
components: [universe]
suites: [jammy-updates]
v1-public-keys: [test-key, extra-key]
+ priority: 10
v1-public-keys:
extra-key:
id: ` + extraTestKey.ID + `
@@ -906,14 +951,13 @@ var setupTests = []setupTest{{
`,
},
release: &setup.Release{
- DefaultArchive: "foo",
-
Archives: map[string]*setup.Archive{
"foo": {
Name: "foo",
Version: "22.04",
Suites: []string{"jammy"},
Components: []string{"main", "universe"},
+ Priority: 20,
PubKeys: []*packet.PublicKey{extraTestKey.PubKey},
},
"bar": {
@@ -921,15 +965,15 @@ var setupTests = []setupTest{{
Version: "22.04",
Suites: []string{"jammy-updates"},
Components: []string{"universe"},
+ Priority: 10,
PubKeys: []*packet.PublicKey{testKey.PubKey, extraTestKey.PubKey},
},
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "foo",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
- Slices: map[string]*setup.Slice{},
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
+ Slices: map[string]*setup.Slice{},
},
},
},
@@ -1040,9 +1084,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"jq": {
- Archive: "ubuntu",
- Name: "jq",
- Path: "slices/mydir/jq.yaml",
+ Name: "jq",
+ Path: "slices/mydir/jq.yaml",
Slices: map[string]*setup.Slice{
"bins": {
Package: "jq",
@@ -1093,9 +1136,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"slice1": {
Package: "mypkg",
@@ -1163,9 +1205,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"slice1": {
Package: "mypkg",
@@ -1186,9 +1227,8 @@ var setupTests = []setupTest{{
},
},
"myotherpkg": {
- Archive: "ubuntu",
- Name: "myotherpkg",
- Path: "slices/mydir/myotherpkg.yaml",
+ Name: "myotherpkg",
+ Path: "slices/mydir/myotherpkg.yaml",
Slices: map[string]*setup.Slice{
"slice1": {
Package: "myotherpkg",
@@ -1310,6 +1350,15 @@ var setupTests = []setupTest{{
`,
},
relerror: `slices test-package_myslice1 and test-package_myslice2 conflict on /dir/\*\* and /dir/file`,
+}, {
+ summary: "Pinned archive is not defined",
+ input: map[string]string{
+ "slices/test-package.yaml": `
+ package: test-package
+ archive: non-existing
+ `,
+ },
+ relerror: `slices/test-package.yaml: package refers to undefined archive "non-existing"`,
}, {
summary: "Specify generate: manifest",
input: map[string]string{
@@ -1335,9 +1384,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -1385,9 +1433,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -1477,9 +1524,8 @@ var setupTests = []setupTest{{
},
Packages: map[string]*setup.Package{
"mypkg": {
- Archive: "ubuntu",
- Name: "mypkg",
- Path: "slices/mydir/mypkg.yaml",
+ Name: "mypkg",
+ Path: "slices/mydir/mypkg.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg",
@@ -1491,9 +1537,8 @@ var setupTests = []setupTest{{
},
},
"mypkg2": {
- Archive: "ubuntu",
- Name: "mypkg2",
- Path: "slices/mydir/mypkg2.yaml",
+ Name: "mypkg2",
+ Path: "slices/mydir/mypkg2.yaml",
Slices: map[string]*setup.Slice{
"myslice": {
Package: "mypkg2",
diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go
index 89c7abc..04be656 100644
--- a/internal/slicer/slicer.go
+++ b/internal/slicer/slicer.go
@@ -88,21 +88,16 @@ func Run(options *RunOptions) error {
targetDir = filepath.Join(dir, targetDir)
}
+ archives, err := selectPkgArchives(options.Archives, options.Selection)
+ if err != nil {
+ return err
+ }
+
// Build information to process the selection.
extract := make(map[string]map[string][]deb.ExtractInfo)
- archives := make(map[string]archive.Archive)
for _, slice := range options.Selection.Slices {
extractPackage := extract[slice.Package]
if extractPackage == nil {
- archiveName := options.Selection.Release.Packages[slice.Package].Archive
- archive := options.Archives[archiveName]
- if archive == nil {
- return fmt.Errorf("archive %q not defined", archiveName)
- }
- if !archive.Exists(slice.Package) {
- return fmt.Errorf("slice package %q missing from archive", slice.Package)
- }
- archives[slice.Package] = archive
extractPackage = make(map[string][]deb.ExtractInfo)
extract[slice.Package] = extractPackage
}
@@ -435,6 +430,55 @@ func createFile(targetPath string, pathInfo setup.PathInfo) (*fsutil.Entry, erro
})
}
+// selectPkgArchives selects the highest priority archive containing the
+// package, unless a particular archive is pinned within the slice definition
+// file. It returns a map of archives indexed by package names.
+func selectPkgArchives(archives map[string]archive.Archive, selection *setup.Selection) (map[string]archive.Archive, error) {
+ sortedArchives := make([]*setup.Archive, 0, len(selection.Release.Archives))
+ for _, archive := range selection.Release.Archives {
+ if archive.Priority < 0 {
+ // Ignore negative priority archives unless a package specifically
+ // asks for it with the "archive" field.
+ continue
+ }
+ sortedArchives = append(sortedArchives, archive)
+ }
+ slices.SortFunc(sortedArchives, func(a, b *setup.Archive) int {
+ return b.Priority - a.Priority
+ })
+
+ pkgArchives := make(map[string]archive.Archive)
+ for _, s := range selection.Slices {
+ if _, ok := pkgArchives[s.Package]; ok {
+ continue
+ }
+ pkg := selection.Release.Packages[s.Package]
+
+ var candidates []*setup.Archive
+ if pkg.Archive == "" {
+ candidates = sortedArchives
+ } else {
+ candidates = []*setup.Archive{selection.Release.Archives[pkg.Archive]}
+ }
+
+ // If the package has not pinned any archive, choose the highest
+ // priority archive in which the package exists.
+ var chosen archive.Archive
+ for _, archiveInfo := range candidates {
+ archive := archives[archiveInfo.Name]
+ if archive != nil && archive.Exists(pkg.Name) {
+ chosen = archive
+ break
+ }
+ }
+ if chosen == nil {
+ return nil, fmt.Errorf("cannot find package %q in archive(s)", pkg.Name)
+ }
+ pkgArchives[pkg.Name] = chosen
+ }
+ return pkgArchives, nil
+}
+
type generateManifestsOptions struct {
packageInfo []*archive.PackageInfo
selection []*setup.Slice
diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go
index faf6b1c..d31be53 100644
--- a/internal/slicer/slicer_test.go
+++ b/internal/slicer/slicer_test.go
@@ -7,6 +7,7 @@ import (
"os"
"path"
"path/filepath"
+ "slices"
"sort"
"strings"
@@ -28,7 +29,7 @@ type slicerTest struct {
summary string
arch string
release map[string]string
- pkgs map[string]testutil.TestPackage
+ pkgs []*testutil.TestPackage
slices []setup.SliceKey
hackopt func(c *C, opts *slicer.RunOptions)
filesystem map[string]string
@@ -247,12 +248,11 @@ var slicerTests = []slicerTest{{
}, {
summary: "Copyright is installed",
slices: []setup.SliceKey{{"test-package", "myslice"}},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- // Add the copyright entries to the package.
- Data: testutil.MustMakeDeb(append(testutil.TestPackageEntries, testPackageCopyrightEntries...)),
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ // Add the copyright entries to the package.
+ Data: testutil.MustMakeDeb(append(testutil.TestPackageEntries, testPackageCopyrightEntries...)),
+ }},
release: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
@@ -280,14 +280,13 @@ var slicerTests = []slicerTest{{
slices: []setup.SliceKey{
{"test-package", "myslice"},
{"other-package", "myslice"}},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- Data: testutil.PackageData["test-package"],
- },
- "other-package": {
- Data: testutil.PackageData["other-package"],
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.PackageData["test-package"],
+ }, {
+ Name: "other-package",
+ Data: testutil.PackageData["other-package"],
+ }},
release: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
@@ -324,19 +323,18 @@ var slicerTests = []slicerTest{{
slices: []setup.SliceKey{
{"implicit-parent", "myslice"},
{"explicit-dir", "myslice"}},
- pkgs: map[string]testutil.TestPackage{
- "implicit-parent": {
- Data: testutil.MustMakeDeb([]testutil.TarEntry{
- testutil.Dir(0755, "./dir/"),
- testutil.Reg(0644, "./dir/file", "random"),
- }),
- },
- "explicit-dir": {
- Data: testutil.MustMakeDeb([]testutil.TarEntry{
- testutil.Dir(01777, "./dir/"),
- }),
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "implicit-parent",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Dir(0755, "./dir/"),
+ testutil.Reg(0644, "./dir/file", "random"),
+ }),
+ }, {
+ Name: "explicit-dir",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Dir(01777, "./dir/"),
+ }),
+ }},
release: map[string]string{
"slices/mydir/implicit-parent.yaml": `
package: implicit-parent
@@ -366,14 +364,13 @@ var slicerTests = []slicerTest{{
slices: []setup.SliceKey{
{"test-package", "myslice"},
{"other-package", "myslice"}},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- Data: testutil.PackageData["test-package"],
- },
- "other-package": {
- Data: testutil.PackageData["other-package"],
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.PackageData["test-package"],
+ }, {
+ Name: "other-package",
+ Data: testutil.PackageData["other-package"],
+ }},
release: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
@@ -704,14 +701,13 @@ var slicerTests = []slicerTest{{
}, {
summary: "Duplicate copyright symlink is ignored",
slices: []setup.SliceKey{{"copyright-symlink-openssl", "bins"}},
- pkgs: map[string]testutil.TestPackage{
- "copyright-symlink-openssl": {
- Data: testutil.MustMakeDeb(packageEntries["copyright-symlink-openssl"]),
- },
- "copyright-symlink-libssl3": {
- Data: testutil.MustMakeDeb(packageEntries["copyright-symlink-libssl3"]),
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "copyright-symlink-openssl",
+ Data: testutil.MustMakeDeb(packageEntries["copyright-symlink-openssl"]),
+ }, {
+ Name: "copyright-symlink-libssl3",
+ Data: testutil.MustMakeDeb(packageEntries["copyright-symlink-libssl3"]),
+ }},
release: map[string]string{
"slices/mydir/copyright-symlink-libssl3.yaml": `
package: copyright-symlink-libssl3
@@ -769,8 +765,88 @@ var slicerTests = []slicerTest{{
},
error: `slice test-package_myslice: content is not a file: /x/y`,
}, {
- summary: "Non-default archive",
+ summary: "Multiple archives with priority",
+ slices: []setup.SliceKey{{"test-package", "myslice"}, {"other-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from foo"),
+ }),
+ Archives: []string{"foo"},
+ }, {
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from bar"),
+ }),
+ Archives: []string{"bar"},
+ }, {
+ Name: "other-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./other-file", "from bar"),
+ }),
+ Archives: []string{"bar"},
+ }},
+ release: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ priority: 20
+ v1-public-keys: [test-key]
+ bar:
+ version: 22.04
+ components: [main]
+ default: true
+ priority: 10
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/test-package.yaml": `
+ package: test-package
+ slices:
+ myslice:
+ contents:
+ /file:
+ `,
+ "slices/mydir/other-package.yaml": `
+ package: other-package
+ slices:
+ myslice:
+ contents:
+ /other-file:
+ `,
+ },
+ filesystem: map[string]string{
+ // The notion of "default" is obsolete and highest priority is selected.
+ "/file": "file 0644 7a3e00f5",
+ // Fetched from archive "bar" as no other archive has the package.
+ "/other-file": "file 0644 fa0c9cdb",
+ },
+ manifestPaths: map[string]string{
+ "/file": "file 0644 7a3e00f5 {test-package_myslice}",
+ "/other-file": "file 0644 fa0c9cdb {other-package_myslice}",
+ },
+}, {
+ summary: "Pinned non-default archive",
slices: []setup.SliceKey{{"test-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from foo"),
+ }),
+ Archives: []string{"foo"},
+ }, {
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from bar"),
+ }),
+ Archives: []string{"bar"},
+ }},
release: map[string]string{
"chisel.yaml": `
format: chisel-v1
@@ -779,10 +855,12 @@ var slicerTests = []slicerTest{{
version: 22.04
components: [main, universe]
default: true
+ priority: 20
v1-public-keys: [test-key]
bar:
version: 22.04
components: [main]
+ priority: 10
v1-public-keys: [test-key]
v1-public-keys:
test-key:
@@ -795,19 +873,169 @@ var slicerTests = []slicerTest{{
slices:
myslice:
contents:
- /dir/nested/file:
+ /file:
`,
},
hackopt: func(c *C, opts *slicer.RunOptions) {
delete(opts.Archives, "foo")
},
filesystem: map[string]string{
- "/dir/": "dir 0755",
- "/dir/nested/": "dir 0755",
- "/dir/nested/file": "file 0644 84237a05",
+ // test-package fetched from pinned archive "bar".
+ "/file": "file 0644 fa0c9cdb",
},
manifestPaths: map[string]string{
- "/dir/nested/file": "file 0644 84237a05 {test-package_myslice}",
+ "/file": "file 0644 fa0c9cdb {test-package_myslice}",
+ },
+}, {
+ summary: "Pinned archive does not have the package",
+ slices: []setup.SliceKey{{"test-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from foo"),
+ }),
+ Archives: []string{"foo"},
+ }},
+ release: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ default: true
+ priority: 20
+ v1-public-keys: [test-key]
+ bar:
+ version: 22.04
+ components: [main]
+ priority: 10
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/test-package.yaml": `
+ package: test-package
+ archive: bar
+ slices:
+ myslice:
+ contents:
+ /file:
+ `,
+ },
+ // Although archive "foo" does have the package, since archive "bar" has
+ // been pinned in the slice definition, no other archives will be checked.
+ error: `cannot find package "test-package" in archive\(s\)`,
+}, {
+ summary: "No archives have the package",
+ slices: []setup.SliceKey{{"test-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{},
+ release: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ default: true
+ priority: 20
+ v1-public-keys: [test-key]
+ bar:
+ version: 22.04
+ components: [main]
+ priority: 10
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/test-package.yaml": `
+ package: test-package
+ slices:
+ myslice:
+ contents:
+ /file:
+ `,
+ },
+ error: `cannot find package "test-package" in archive\(s\)`,
+}, {
+ summary: "Negative priority archives are ignored when not explicitly pinned in package",
+ slices: []setup.SliceKey{{"test-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from foo"),
+ }),
+ Archives: []string{"foo"},
+ }},
+ release: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ default: true
+ priority: -20
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/test-package.yaml": `
+ package: test-package
+ slices:
+ myslice:
+ contents:
+ /file:
+ `,
+ },
+ // Although test-package exists in archive "foo", the archive was ignored
+ // due to having a negative priority.
+ error: `cannot find package "test-package" in archive\(s\)`,
+}, {
+ summary: "Negative priority archive explicitly pinned in package",
+ slices: []setup.SliceKey{{"test-package", "myslice"}},
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ testutil.Reg(0644, "./file", "from foo"),
+ }),
+ Archives: []string{"foo"},
+ }},
+ release: map[string]string{
+ "chisel.yaml": `
+ format: chisel-v1
+ archives:
+ foo:
+ version: 22.04
+ components: [main, universe]
+ default: true
+ priority: -20
+ v1-public-keys: [test-key]
+ v1-public-keys:
+ test-key:
+ id: ` + testKey.ID + `
+ armor: |` + "\n" + testutil.PrefixEachLine(testKey.PubKeyArmor, "\t\t\t\t\t\t") + `
+ `,
+ "slices/mydir/test-package.yaml": `
+ package: test-package
+ archive: foo
+ slices:
+ myslice:
+ contents:
+ /file:
+ `,
+ },
+ filesystem: map[string]string{
+ "/file": "file 0644 7a3e00f5",
+ },
+ manifestPaths: map[string]string{
+ "/file": "file 0644 7a3e00f5 {test-package_myslice}",
},
}, {
summary: "Multiple slices of same package",
@@ -1068,22 +1296,19 @@ var slicerTests = []slicerTest{{
{"test-package", "myslice"},
{"other-package", "myslice"},
},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- Name: "test-package",
- Hash: "h1",
- Version: "v1",
- Arch: "a1",
- Data: testutil.PackageData["test-package"],
- },
- "other-package": {
- Name: "other-package",
- Hash: "h2",
- Version: "v2",
- Arch: "a2",
- Data: testutil.PackageData["other-package"],
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Hash: "h1",
+ Version: "v1",
+ Arch: "a1",
+ Data: testutil.PackageData["test-package"],
+ }, {
+ Name: "other-package",
+ Hash: "h2",
+ Version: "v2",
+ Arch: "a2",
+ Data: testutil.PackageData["other-package"],
+ }},
release: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
@@ -1107,22 +1332,19 @@ var slicerTests = []slicerTest{{
slices: []setup.SliceKey{
{"test-package", "myslice"},
},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- Name: "test-package",
- Hash: "h1",
- Version: "v1",
- Arch: "a1",
- Data: testutil.PackageData["test-package"],
- },
- "other-package": {
- Name: "other-package",
- Hash: "h2",
- Version: "v2",
- Arch: "a2",
- Data: testutil.PackageData["other-package"],
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Hash: "h1",
+ Version: "v1",
+ Arch: "a1",
+ Data: testutil.PackageData["test-package"],
+ }, {
+ Name: "other-package",
+ Hash: "h2",
+ Version: "v2",
+ Arch: "a2",
+ Data: testutil.PackageData["other-package"],
+ }},
release: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
@@ -1143,19 +1365,18 @@ var slicerTests = []slicerTest{{
}, {
summary: "Relative paths are properly trimmed during extraction",
slices: []setup.SliceKey{{"test-package", "myslice"}},
- pkgs: map[string]testutil.TestPackage{
- "test-package": {
- Data: testutil.MustMakeDeb([]testutil.TarEntry{
- // This particular path starting with "/foo" is chosen to test for
- // a particular bug; which appeared due to the usage of
- // strings.TrimLeft() instead strings.TrimPrefix() to determine a
- // relative path. Since TrimLeft takes in a cutset instead of a
- // prefix, the desired relative path was not produced.
- // See https://github.com/canonical/chisel/pull/145.
- testutil.Dir(0755, "./foo-bar/"),
- }),
- },
- },
+ pkgs: []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.MustMakeDeb([]testutil.TarEntry{
+ // This particular path starting with "/foo" is chosen to test for
+ // a particular bug; which appeared due to the usage of
+ // strings.TrimLeft() instead strings.TrimPrefix() to determine a
+ // relative path. Since TrimLeft takes in a cutset instead of a
+ // prefix, the desired relative path was not produced.
+ // See https://github.com/canonical/chisel/pull/145.
+ testutil.Dir(0755, "./foo-bar/"),
+ }),
+ }},
hackopt: func(c *C, opts *slicer.RunOptions) {
opts.TargetDir = filepath.Join(filepath.Clean(opts.TargetDir), "foo")
err := os.Mkdir(opts.TargetDir, 0755)
@@ -1232,25 +1453,17 @@ func (s *S) TestRun(c *C) {
func runSlicerTests(c *C, tests []slicerTest) {
for _, test := range tests {
- for _, slices := range testutil.Permutations(test.slices) {
+ for _, testSlices := range testutil.Permutations(test.slices) {
c.Logf("Summary: %s", test.summary)
if _, ok := test.release["chisel.yaml"]; !ok {
test.release["chisel.yaml"] = defaultChiselYaml
}
if test.pkgs == nil {
- test.pkgs = map[string]testutil.TestPackage{
- "test-package": {
- Data: testutil.PackageData["test-package"],
- },
- }
- }
- for pkgName, pkg := range test.pkgs {
- if pkg.Name == "" {
- // We need to add the name for the manifest validation.
- pkg.Name = pkgName
- test.pkgs[pkgName] = pkg
- }
+ test.pkgs = []*testutil.TestPackage{{
+ Name: "test-package",
+ Data: testutil.PackageData["test-package"],
+ }}
}
releaseDir := c.MkDir()
@@ -1280,16 +1493,22 @@ func runSlicerTests(c *C, tests []slicerTest) {
},
Scripts: setup.SliceScripts{},
}
- slices = append(slices, setup.SliceKey{
+ testSlices = append(testSlices, setup.SliceKey{
Package: manifestPackage,
Slice: "manifest",
})
- selection, err := setup.Select(release, slices)
+ selection, err := setup.Select(release, testSlices)
c.Assert(err, IsNil)
archives := map[string]archive.Archive{}
for name, setupArchive := range release.Archives {
+ pkgs := make(map[string]*testutil.TestPackage)
+ for _, pkg := range test.pkgs {
+ if len(pkg.Archives) == 0 || slices.Contains(pkg.Archives, name) {
+ pkgs[pkg.Name] = pkg
+ }
+ }
archive := &testutil.TestArchive{
Opts: archive.Options{
Label: setupArchive.Name,
@@ -1298,7 +1517,7 @@ func runSlicerTests(c *C, tests []slicerTest) {
Components: setupArchive.Components,
Arch: test.arch,
},
- Packages: test.pkgs,
+ Packages: pkgs,
}
archives[name] = archive
}
diff --git a/internal/testutil/archive.go b/internal/testutil/archive.go
index 77b945c..ae8258c 100644
--- a/internal/testutil/archive.go
+++ b/internal/testutil/archive.go
@@ -10,15 +10,16 @@ import (
type TestArchive struct {
Opts archive.Options
- Packages map[string]TestPackage
+ Packages map[string]*TestPackage
}
type TestPackage struct {
- Name string
- Version string
- Hash string
- Arch string
- Data []byte
+ Name string
+ Version string
+ Hash string
+ Arch string
+ Data []byte
+ Archives []string
}
func (a *TestArchive) Options() *archive.Options { |
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.
A couple more comments about the testing part of this PR. This is looking very good Rafid. The only thing pending is to have a proper diff in Github against main and to give it the final review.
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.
Amazing work Rafid, only a few minor comments and we are good to go. Thanks!
This commit deprecates the concept of default archive in chisel.yaml. In essence, it deprecates the "archives.<archive>.default" field from the chisel.yaml configurations. However, the field can still be present and it will be parsed but IGNORED. (See setup_test.go) DEPRECATED: "archives.<archive>.default" field in chisel.yaml.
There were some usage of the default field in archives left behind. This commit cleans them up.
This PR adds support for fetching packages from multiple archives. It introduces a new field
archives.<archive-name>.priority
which takes in a signed integer and specifies the priority of a certain archive. This value is particularly useful when there are multiple archives. A package is fetched from the archive with highest priority regardless of the "default" archive, unless the slice definition file of that package specifies a particular archive using the "archive" field.Example chisel.yaml:
In the above example, if a package exists in both of the archives, the package will be fetched from archive "foo" since it has higher priority. Note that, archive "bar" is the default. However, if in a particular package such as below, the "archive" field is used to specify a particular archive, that particular archive will be fetched from.
The above package will be fetched from archive "bar", regardless of priority.
Reference: Specification RK018.