Add support for pro archives #53
Conversation
Do not tie packages to individual archives and pick the package with highest version from all defined archives.
woky
force-pushed
the
fips2
branch
2 times, most recently
from
d6cbd4a to
2b18c18
Compare
And also run go mod tidy
woky
force-pushed
the
fips2
branch
2 times, most recently
from
08ce9bd to
b5b3396
Compare
There was a problem hiding this comment.
Per early conversations around collaboration, this PR needs to be broken down into finer grained changes so we can review and merge them more timely. The description itself already makes it clear that this is several independent changes bundled into one, which is a pattern that creates very long PRs, very long reviews, and frustration because it takes a many roundtrips of independent reviews until we can merge any one of the changes. On the other hands, with smaller and to-the-point PRs, we can discuss the issue at hand, adapt, and merge.
|
I'm gonna finish review with Cris in this PR and then I'll split it. |
There was a problem hiding this comment.
Nice! I looked through the changes and have a few suggestions.
I too think this should be split into a few more PRs, specially the Go version upgrade one. Additionally, I think it can be further split into the non-Pro archive updates such as having multiple archives, priority, not tying packages to archive, and the new Pro archive features. Of course, the PR/commit with the Pro feature will be depending on the non-Pro updates PR/commit.
| Exists(pkg string) bool | ||
| Version(pkg string) string |
There was a problem hiding this comment.
I think we should leave Exists as it is, and just add another function called Version.
They might just do the same thing as it seems, but it feels a bit weird to check if package is there by checking it's version, which I realize now is what's being done inside selectPackage itself, but still Exists seems intuitive.
There was a problem hiding this comment.
But the code where Exists was used doesn't exist anymore. And if I did Exists followed by Version it'd be two lookups instead of one.
| func (a *ubuntuArchive) Exists(pkg string) bool { | ||
| _, _, err := a.selectPackage(pkg) | ||
| return err == nil | ||
| func (a *ubuntuArchive) Version(pkg string) string { | ||
| _, _, version := a.selectPackage(pkg) | ||
| return version | ||
| } |
There was a problem hiding this comment.
Same comment as above. Let's keep both I think.
There was a problem hiding this comment.
See my comment above. The Exists would be unused.
| func (a *ubuntuArchive) selectPackage(pkg string) (control.Section, *ubuntuIndex, error) { | ||
| func (a *ubuntuArchive) selectPackage(pkg string) (control.Section, *ubuntuIndex, string) { |
There was a problem hiding this comment.
Similarly, I think we can keep this signature same and return an error like before as the third return value. We can deduce the Version from control.Section that will be returned as the first return value, e.g.
section, _, err := a.selectPackage(pkg)
if err != nil {
...
}
version := section.Get("Version")There was a problem hiding this comment.
And section.Get parses the section text every time it's called. Se when we get the version inside selectPackage, why parse the section block again right after that?
I don't understand the intention. This is internal function used only few lines above and below.
Co-authored-by: Rafid Bin Mostofa <[email protected]>
|
My understanding is that this PR is being broken down into individual PRs at the moment, so I'll go ahead and close it so we don't mix it up. |
This PR adds support for FIPS repositories along with preparatory changes:
go mod tidy. Some new code useserrors.Join./etc/apt/auth.conf.d/. This is not enabled by default, see below.Currently, fips and fips-updates exists only for focal for which we don't have chisel-releases yet. I've created a fork with reduced set of slices. You can try this implementation with it provided you have pro fips-updates credentials available on your machine.