Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: support multiple archives with "priority" field #160

Merged
merged 13 commits into from
Oct 18, 2024
+831 −247
Merged

feat: support multiple archives with "priority" field #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
64d1e74
feat: support multiple archives with "priority" field (#12)
rebornplusplus Oct 3, 2024
82587f6
Merge branch 'main' into multiple-archive-support
letFunny Oct 7, 2024
39c3023
Merge branch 'main' into multiple-archive-support
letFunny Oct 14, 2024
a19c876
more descriptive variable name
letFunny Oct 14, 2024
bb18527
pkgToArchive -> pkgArchive
letFunny Oct 14, 2024
a8cdd86
respect default archive if priorities not being used
letFunny Oct 15, 2024
40d0867
simplify for loop, unnecessary variable
letFunny Oct 15, 2024
7d1e3f0
remove unnecessary bool flag
letFunny Oct 16, 2024
1183226
make tests deterministic
letFunny Oct 16, 2024
bb5f58b
make more tests deterministic
letFunny Oct 16, 2024
4962b27
Trigger CI
letFunny Oct 17, 2024
418ea0d
fix: empty priority is no longer valid
letFunny Oct 18, 2024
c52173a
fix: priority cannot be 0
letFunny Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions cmd/chisel/cmd_find_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ func makeSamplePackage(pkg string, slices []string) *setup.Package {
}

var sampleRelease = &setup.Release{
DefaultArchive: "ubuntu",

Archives: map[string]*setup.Archive{
"ubuntu": {
Name: "ubuntu",
Expand Down
8 changes: 0 additions & 8 deletions cmd/chisel/cmd_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg1_myslice1"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand All @@ -37,7 +36,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg2"},
stdout: `
package: mypkg2
archive: ubuntu
slices:
myslice:
contents:
Expand All @@ -49,7 +47,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg1_myslice2", "mypkg1_myslice1"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand All @@ -65,7 +62,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg1_myslice1", "mypkg2", "mypkg1_myslice2"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand All @@ -76,7 +72,6 @@ var infoTests = []infoTest{{
- mypkg2_myslice
---
package: mypkg2
archive: ubuntu
slices:
myslice:
contents:
Expand All @@ -88,7 +83,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg1_myslice1", "mypkg1"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand All @@ -104,7 +98,6 @@ var infoTests = []infoTest{{
query: []string{"mypkg1_myslice1", "mypkg1_myslice1", "mypkg1_myslice1"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand All @@ -121,7 +114,6 @@ var infoTests = []infoTest{{
query: []string{"foo", "mypkg1_myslice1", "bar_foo"},
stdout: `
package: mypkg1
archive: ubuntu
slices:
myslice1:
contents:
Expand Down
88 changes: 76 additions & 12 deletions internal/setup/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ import (
// Release is a collection of package slices targeting a particular
// distribution version.
type Release struct {
Path string
Packages map[string]*Package
Archives map[string]*Archive
DefaultArchive string
letFunny marked this conversation as resolved.
Show resolved Hide resolved
Path string
Packages map[string]*Package
Archives map[string]*Archive
}

// Archive is the location from which binary packages are obtained.
Expand All @@ -33,6 +32,7 @@ type Archive struct {
Version string
Suites []string
Components []string
Priority int
PubKeys []*packet.PublicKey
}

Expand Down Expand Up @@ -229,6 +229,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 %d", 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
}

Expand Down Expand Up @@ -355,9 +377,6 @@ func readSlices(release *Release, baseDir, dirName string) error {
if err != nil {
return err
}
if pkg.Archive == "" {
pkg.Archive = release.DefaultArchive
letFunny marked this conversation as resolved.
Show resolved Hide resolved
}

release.Packages[pkg.Name] = pkg
}
Expand All @@ -370,10 +389,16 @@ type yamlRelease struct {
PubKeys map[string]yamlPubKey `yaml:"public-keys"`
}

const (
MaxArchivePriority = 1000
MinArchivePriority = -1000
)

type yamlArchive struct {
Version string `yaml:"version"`
Suites []string `yaml:"suites"`
Components []string `yaml:"components"`
Priority *int `yaml:"priority"`
Default bool `yaml:"default"`
PubKeys []string `yaml:"public-keys"`
}
Expand Down Expand Up @@ -513,6 +538,11 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
pubKeys[keyName] = key
}

// For compatibility if there is a default archive set and priorities are
// not being used, we will revert back to the default archive behaviour.
hasPriority := false
var defaultArchive string
var archiveNoPriority string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noPriorityArchive would be friends with defaultArchive :)

for archiveName, details := range yamlVar.Archives {
if details.Version == "" {
return nil, fmt.Errorf("%s: archive %q missing version field", fileName, archiveName)
Expand All @@ -523,13 +553,14 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
if len(details.Components) == 0 {
return nil, fmt.Errorf("%s: archive %q missing components field", fileName, archiveName)
}
if len(yamlVar.Archives) == 1 {
details.Default = true
} else if details.Default && release.DefaultArchive != "" {
return nil, fmt.Errorf("%s: more than one default archive: %s, %s", fileName, release.DefaultArchive, archiveName)
letFunny marked this conversation as resolved.
Show resolved Hide resolved
if details.Default && defaultArchive != "" {
if archiveName < defaultArchive {
archiveName, defaultArchive = defaultArchive, archiveName
}
return nil, fmt.Errorf("%s: more than one default archive: %s, %s", fileName, defaultArchive, archiveName)
}
if details.Default {
release.DefaultArchive = archiveName
defaultArchive = archiveName
}
if len(details.PubKeys) == 0 {
return nil, fmt.Errorf("%s: archive %q missing public-keys field", fileName, archiveName)
Expand All @@ -542,14 +573,47 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
}
archiveKeys = append(archiveKeys, key)
}
priority := 0
if details.Priority != nil {
hasPriority = true
Copy link
Contributor

@niemeyer niemeyer Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handling of priority zero seems a bit loose at the moment. We consider it not being zero here as "it has priority", but then right below we actually define the default (!) priority as zero. Also, we error if two priorities are the same, which forces people to enter a priority value for all but one of them, which will be automatically set to zero due to how yaml unmarshaling takes place on the int type.

It looks like we might fix all of these issues at once by forcing the priority, if present, to be either positive or negative, and considering zero to be unset per the test above. On unmarshalling, we can clarify the situation further by having the Priority field on the yaml type being a pointer, so we can differentiate between unset and zero, and then failing on inconsistency (any priority is set but some not, or no default and no priority).

Copy link
Collaborator Author

@letFunny letFunny Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 418ea0d, I have changed the priority to be a pointer so we can distinguish when it is 0 and when it is unset. Also, we now return an error in the case there is an inconsistent use of priorities. The only bit I am not sure about is the error message when there are no priorities set across several archives (I have included a test in the commit to show that use-case).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error we want in those cases is to point out just one of them as a hint. Something like:

error: archive %q is missing the priority setting

We can raise this when there is no defaultArchive, and we have a priorityArchive (which needs support, equivalent to noPriorityArchive).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. We know check if there is more than one archive and there is no default and there are no priorities.

priority = *details.Priority
if priority > MaxArchivePriority || priority < MinArchivePriority || priority == 0 {
return nil, fmt.Errorf("%s: archive %q has invalid priority value of %d", fileName, archiveName, priority)
}
} else {
if archiveNoPriority == "" || archiveName < archiveNoPriority {
// Make it deterministic.
archiveNoPriority = archiveName
}
}
release.Archives[archiveName] = &Archive{
Name: archiveName,
Version: details.Version,
Suites: details.Suites,
Components: details.Components,
Priority: priority,
PubKeys: archiveKeys,
}
}
if (hasPriority && archiveNoPriority != "") ||
(!hasPriority && defaultArchive == "" && len(yamlVar.Archives) > 1) {
return nil, fmt.Errorf("%s: archive %q is missing the priority setting", fileName, archiveNoPriority)
}
if defaultArchive != "" && !hasPriority {
// For compatibility with the default archive behaviour we will set
// negative priorities to all but the default one, which means all
// others will be ignored unless pinned.
var archiveNames []string
for archiveName := range yamlVar.Archives {
archiveNames = append(archiveNames, archiveName)
}
// Make it deterministic.
slices.Sort(archiveNames)
for i, archiveName := range archiveNames {
release.Archives[archiveName].Priority = -i - 1
}
release.Archives[defaultArchive].Priority = 1
}

return release, err
}
Expand Down
Loading
Loading