Repository navigation
feat: support multiple archives with "priority" field - #160
Conversation
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. A package is fetched from the archive with highest priority. It also deprecates the concept of default archive in chisel.yaml. However, the field can still be present and it will be parsed but IGNORED. DEPRECATED: "archives.<archive>.default" field in chisel.yaml. --------- Co-authored-by: Alberto Carretero <[email protected]>
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Looking good! Mostly trivial comments.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
It's looking good. Just one detail about zero priorities being slightly loose.
| return nil, fmt.Errorf("%s: archive %q has invalid priority value of %d", fileName, archiveName, details.Priority) | ||
| } | ||
| if details.Priority != 0 { | ||
| hasPriority = true |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Done. We know check if there is more than one archive and there is no default and there are no priorities.
| } | ||
| } else { | ||
| if hasPriority { | ||
| return nil, fmt.Errorf("%s: archive %q missing priority", fileName, archiveName) |
There was a problem hiding this comment.
We probably don't need the criss-cross check and double error message, if we move this to a single check at the end, where we're already verifying priorities anyway at line 601:
if hasPriority {
if archiveNoPriority {
... error ...
}
} else if defaultArchive {
...
}
We might also turn the branch here in line 586 into:
} else if archiveNoPriority == "" {
archiveNoPriority = archiveName
}
So we only pick up the first one.
There was a problem hiding this comment.
Agree with the suggestions. The only thing I have changed is to make the branch in line 586 deterministic by ordering the archive names.
| // not being used, we will revert back to the default archive behaviour. | ||
| hasPriority := false | ||
| var defaultArchive string | ||
| var archiveNoPriority string |
There was a problem hiding this comment.
noPriorityArchive would be friends with defaultArchive :)
| return nil, fmt.Errorf("%s: archive %q has invalid priority value of %d", fileName, archiveName, details.Priority) | ||
| } | ||
| if details.Priority != 0 { | ||
| hasPriority = true |
There was a problem hiding this comment.
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).
"v2-archives" defines the archives, same as "archives". It is added to define Ubuntu Pro archives in chisel-releases with "pro" and "priority" fields (see canonical#160 and canonical#167), while supporting Chisel<=v1.0.0 and chisel-releases "format"<=v1. Since Chisel ignores unknown fields, archives defined in "v2-archives" will be ignored by v1.0.0 but picked up by later versions.
This commit adds support for fetching packages from multiple archives. It introduces a new field
archives.<archive-name>.prioritywhich takes in a signed integer and specifies the priority of a certain archive. A package is fetched from the archive with highest priority and negative priorities are ignored unless explicitly pinned in a package.The concept of default archive in chisel.yaml is now deprecated. However, the field can still be present and it will be parsed but IGNORED.
DEPRECATED: "archives..default" field in chisel.yaml.