Add support to mirror non package installer files #680

sliverc · 2017-11-27T09:42:06Z

Fixes #304
Fixes #240

Description of the Change

This PR introduces new -with-installer flag for mirror create. When updating and publishing mirror it will include installer files.

After analysis of Debian and Ubuntu repositories following rules are applied for this to work:

per dists and component an installer-$ARCH folder. (Note: This folder doesn't exist on all components or archs. If not present no error should be raised.)
in this folder there is always a checksum file current/images/SHA256SUMS listing all the installer files
Examples:
https://mirror.yandex.ru/debian/dists/jessie/main/installer-amd64/current/images/SHA256SUMS
https://mirror.yandex.ru/ubuntu/dists/xenial/main/installer-i386/current/images/SHA256SUMS
Debian installer lists this file in the Release file which can be used for checksum check
Ubuntu doesn't list it in Release but provides a separate .gpg file for verification
https://mirror.yandex.ru/ubuntu/dists/xenial/main/installer-i386/current/images/SHA256SUMS.gpg

Checklist

unit-test added (if change is algorithm)
functional test added/updated (if change is functional)
man page updated (if applicable)
bash completion updated (if applicable)
documentation updated
author name in AUTHORS

sliverc · 2017-11-27T13:16:27Z

Documentation at aptly-dev/www.aptly.info#58

gittygoo · 2018-04-13T15:32:40Z

Great feature, lets bring it to production

codecov · 2018-05-17T12:20:29Z

Codecov Report

Merging #680 into master will decrease coverage by 0.04%.
The diff coverage is 66.08%.

@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
- Coverage   64.13%   64.08%   -0.05%     
==========================================
  Files          50       50              
  Lines        6329     6446     +117     
==========================================
+ Hits         4059     4131      +72     
- Misses       1776     1810      +34     
- Partials      494      505      +11

Impacted Files	Coverage Δ
http/compression.go	`86.11% <0%> (ø)`	⬆️
http/http.go	`50% <0%> (-50%)`	⬇️
swift/public.go	`59.37% <100%> (ø)`	⬆️
deb/changes.go	`59.74% <100%> (ø)`	⬆️
files/public.go	`78.62% <100%> (+0.16%)`	⬆️
s3/public.go	`59.22% <100%> (ø)`	⬆️
deb/deb.go	`60.83% <100%> (ø)`	⬆️
deb/index_files.go	`43.26% <50%> (+0.28%)`	⬆️
deb/remote.go	`62.74% <60.37%> (-1.37%)`	⬇️
deb/publish.go	`63.48% <61.9%> (-0.33%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec57d17...d1b2814. Read the comment docs.

sliverc · 2018-05-17T12:28:35Z

@apachelogger I have rebased this PR to current master. Could you review it? Thanks.

hsitter · 2018-05-18T09:49:53Z

First I'll have to read up installer files again, it's been ages since I last dealt with them xD

I'll try to get it done today.

hsitter

It bothers me greatly that the installer metadata is so very randomly put together and doesn't even have proper documentation 😒

https://wiki.debian.org/DebianInstaller/NetbootMetadata proposes a proper format and is a 10 year old page. So I have zero hope for this getting any less awkward in the future.
Anyway, nothing we can do much about unfortunately.

I didn't quite get through everything, and I won't get it to it before Tuesday. So, if @smira wants to review meanwhile I'd have no problem with that.

Do we want to support installer data from our side as well? i.e. on a !mirror repo, should you be able to add installer stuff? I kinda think yes, but then I also don't really care 😉. A cheap way to support this might be via the proposed #473 actually, not sure if we need to do anything to facilitate that though.

The PR does generally look alright. I have a particular gripe with the change in files/public.go though and would like some background info on that + adjustments to s3/swift.

Lastly, iff the remote had detatched signatures on the SUMS files I think we also need to detach the signatures on the SUMS files. From some quick research I did, the entire structure and content of the installer-* directories is entirely dependent on the debian-installer version at hand (in fact, udeb.list apparently changed its format at some point even). So, to actually have this mirrored reliably I think the actual signing specifics need to get repliacted as well.

hsitter · 2018-05-18T12:54:49Z

deb/remote.go

 		if err != nil {
-			return err
+			if _, ok := err.(*http.NoCandidateFoundError); isInstaller && ok {
+				// checking of gpg file is only needed when checksums matches are required.


hsitter · 2018-05-18T14:31:58Z

files/public.go

@@ -123,10 +123,11 @@ func (storage *PublishedStorage) RemoveDirs(path string, progress aptly.Progress
 // sourcePath is a relative path to package file in package pool
 //
 // LinkFromPool returns relative path for the published file to be included in package index
-func (storage *PublishedStorage) LinkFromPool(publishedDirectory, baseName string, sourcePool aptly.PackagePool,
+func (storage *PublishedStorage) LinkFromPool(publishedDirectory, fileName string, sourcePool aptly.PackagePool,


Is this actually related?

It's missing a unit test.
It also seems inconsistent with s3 and swift, both still call it baseName.
Talking about s3 and swift, do they need changing (other than that the var name should be the same)?

I think my intention was to keep directory & file name (without any paths) as separate args, as directory might need to be created. I'm not sure what we get if we allow paths in baseName

Up to now the files in the published folder were flat but with installer files can be nested.

The only difference is that creating of directories needs to be handled differently which this changes addresses here. For s3/swift there is no difference as directory creation does not matter there.

I have adjusted all LinkFromPool params in s3/swift to be named fileName to clarify that it can contain slashes (even though implementation doesn't change). Also added a test so this is actually proven.

hsitter · 2018-05-18T14:47:17Z

deb/format.go

 }

 // NewControlFileReader creates ControlFileReader, it wraps with buffering
-func NewControlFileReader(r io.Reader) *ControlFileReader {
+func NewControlFileReader(r io.Reader, isRelease, isInstaller bool) *ControlFileReader {


So, I am not sure how iditomatic this is, but I feel something like

const ( IsRelease = iota IsInstaller = 1 << iota )

used like NewControlFileReader(reader, IsInstaller | IsRelease) would be vastly easier to read than the distinctly unexpressive NewControlFileReader(reader, true, false).

That said, I've never tried anything like that, so I couldn't say how well this works in practise.

I agree that this would be more readable in this certain case. But issue is that flags (meaning whether it is a release, source or installer package) are currently stored as boolean on the level database and therefore simply forwarded e.g. to WriteTo. Simply adjusting this in NewControlFileReader but leaving it in all other cases would make the code inconsistent.

As mentioned above I think we should think about refactoring the code how package index types are handled, to simplify this and potentially also store information of a package as flags. However I think this is out of scope for this PR.

hsitter · 2018-05-18T14:49:32Z

deb/format.go

-	scanner *bufio.Scanner
+	scanner     *bufio.Scanner
+	isRelease   bool
+	isInstaller bool


Is there a reason you didn't write a new reader for the installer metadata? They aren't really deb822 let alone control files, so it seems a bit odd to run them through the control reader.

If we want to make its own reader type for installer package files it would still need to implement ReadStanza but this would be odd as well as it doesn't have any sections.

So instead to not complicate the code (especially DownloadPackageIndex) I handle the file as it would be a control file without any sections. This avoids duplication of code.

To be honest though I think there are too many if/else all over the code in terms of package index type. I think such should be avoided by implementing proper types but this I think is out of scope for this PR.

smira · 2018-05-18T21:24:34Z

I would agree with @apachelogger in the sense that if we support installer, we should support it everywhere, including local repos. I'm not that familiar with this thing to understand what do we actually need to support on our side to make it work properly.

smira · 2018-05-18T21:26:16Z

aptly/interfaces.go

@@ -130,6 +130,8 @@ type Downloader interface {
 	DownloadWithChecksum(ctx context.Context, url string, destination string, expected *utils.ChecksumInfo, ignoreMismatch bool, maxTries int) error
 	// GetProgress returns Progress object
 	GetProgress() Progress
+	// GetLength of given url


I think it should be better to say 'GetLength returns size by heading object with url` ?

It's not len(url) ;)

smira · 2018-05-18T21:30:00Z

deb/publish.go

 			for _, arch := range p.Architectures {
 				if pkg.MatchesArchitecture(arch) {
-					matches = true
+					hadUdebs = hadUdebs || pkg.IsUdeb
+					err = pkg.LinkFromPool(publishedStorage, packagePool, p.Prefix, p.Distribution, component, arch, forceOverwrite)


This seems like overkill to call it len(p.Arhitectures) times if package is not installer package, as each call does Stat for local fs and for S3 it's even more involved.

Not quite sure what you mean. Only LinkFromPool actually accesses storage. But on line 595 there is still a break, so LinkFromPool is only called once.

Could you elaborate a bit more what actually does the Stat here which it didn't before? For me it looks the same just simply the if inlined... but maybe I am overlooking something.

smira · 2018-05-18T21:32:12Z

deb/package.go

@@ -576,6 +621,9 @@ func (p *Package) LinkFromPool(publishedStorage aptly.PublishedStorage, packageP
 		}

 		relPath := filepath.Join("pool", component, poolDir)


should we build relPath outside of LinkFromPool so that we don't have to pass all the params?

e.g. publish.go could have had correct logic for it, or we can make a method of Package which returns correct "pool" path depending on package type?

Something like LinkFromPool(publishedStorage, packagePool, relPath, force) ?

Fair point. I have adjusted it so only a relPath is passed on. I kept prefix though as this is handled the same way in all cases.

sliverc · 2018-05-22T14:50:43Z

For completeness aptly now also creates a detached signature of the installer hash sum file when publishing. This way we should be on the safe side for the installer to work no matter what version.

All other questions and reviews points I should have addressed (see discussion comments), otherwise let me know if there is anything still unclear.

smira

thanks, 👍

one thing which might be good for the future work is to add support for installer file handling in local repos (adding them from local filesystem)

sliverc · 2018-05-28T09:32:31Z

@smira Thanks. Agree that it would be a great addition to add installer files in local repos for the future.

@apachelogger How does it look to you now? (I am not able to merge it even though @smira Accepeted as there are still requesting changes)

Additionally I have added some more unit test to fix codecov checks.

sliverc · 2018-06-19T14:32:03Z

@apachelogger
Do you think you have time to review your requested changes? Even though @smira has accepted, merge is blocked till all reviews are accepted.

I would like to update my other PRs once this is merged. If you do not have any time you can also consider dismissing your review. Thanks.

gittygoo · 2018-06-22T00:22:22Z

Can we get this merged? we are desperately needing it :(

As this has been hanging around for quite a while I am merging this PR as of accpeted review of smira. In case there are still comments, we can always follow up on them later. Just let me know.

Add documentation for aptly-dev/aptly#680

sathieu · 2019-01-22T17:46:02Z

I know it's late in buster release cycle, but:

Is there any chance to release a new aptly version to have this feature in Debian buster?

sliverc force-pushed the with_installer branch from 9ab2be3 to f146e6d Compare November 27, 2017 10:51

sliverc mentioned this pull request Nov 27, 2017

Document mirror -with-installer flag aptly-dev/www.aptly.info#58

Merged

sliverc force-pushed the with_installer branch 2 times, most recently from 7553741 to 08de6af Compare December 1, 2017 08:43

smira added 1.3.0 feature labels Dec 5, 2017

sliverc force-pushed the with_installer branch 2 times, most recently from 00d0f3d to 95d76de Compare December 18, 2017 10:05

hsitter added 1.4.0 and removed 1.3.0 labels Apr 30, 2018

sliverc force-pushed the with_installer branch 2 times, most recently from 56e63ec to 2f212dd Compare May 17, 2018 12:07

sliverc requested a review from a team May 17, 2018 12:27

hsitter previously requested changes May 18, 2018

View reviewed changes

smira reviewed May 18, 2018

View reviewed changes

smira approved these changes May 24, 2018

View reviewed changes

sliverc force-pushed the with_installer branch from c0a8669 to d16d473 Compare May 28, 2018 09:15

sliverc force-pushed the with_installer branch 3 times, most recently from 9b331ae to 768db88 Compare June 18, 2018 07:24

sliverc force-pushed the with_installer branch from 768db88 to 08a434e Compare June 19, 2018 14:08

sliverc added this to the 1.4.0 milestone Jun 19, 2018

Oliver Sauder added 7 commits July 6, 2018 15:02

Add support to mirror non package installer files

108b0ea

Allow editing of with-installer mirror flag

074904e

Clarify doc strings

6df4a74

Simplify package function signature LinkFromPool

0075ead

Test LinkFromPool with nested filenames

2e52692

Add detached signature to installer hashsum file

b7323db

Add unit test for remote and http

b1a2523

sliverc force-pushed the with_installer branch from 08a434e to b1a2523 Compare July 6, 2018 13:02

sliverc added 2 commits September 21, 2018 13:26

Merge branch 'master' into with_installer

e23e30e

Merge branch 'master' into with_installer

d1b2814

sliverc merged commit 702c1ff into aptly-dev:master Sep 27, 2018

sliverc deleted the with_installer branch September 27, 2018 07:50

sliverc added a commit to aptly-dev/www.aptly.info that referenced this pull request Sep 27, 2018

Document mirror -with-installer flag (#58)

3b3cdf9

Add documentation for aptly-dev/aptly#680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to mirror non package installer files #680

Add support to mirror non package installer files #680

sliverc commented Nov 27, 2017 •

edited

Loading

sliverc commented Nov 27, 2017

gittygoo commented Apr 13, 2018

codecov bot commented May 17, 2018 •

edited

Loading

sliverc commented May 17, 2018

hsitter commented May 18, 2018

hsitter left a comment

hsitter May 18, 2018

sliverc May 22, 2018

hsitter May 18, 2018

smira May 18, 2018

sliverc May 22, 2018

hsitter May 18, 2018

sliverc May 22, 2018

hsitter May 18, 2018

sliverc May 22, 2018

smira commented May 18, 2018

smira May 18, 2018

sliverc May 22, 2018

smira May 18, 2018

sliverc May 22, 2018

smira May 18, 2018

sliverc May 22, 2018

sliverc commented May 22, 2018

smira left a comment

sliverc commented May 28, 2018

sliverc commented Jun 19, 2018

gittygoo commented Jun 22, 2018

sathieu commented Jan 22, 2019

		@@ -576,6 +621,9 @@ func (p *Package) LinkFromPool(publishedStorage aptly.PublishedStorage, packageP
		}

		relPath := filepath.Join("pool", component, poolDir)

Add support to mirror non package installer files #680

Add support to mirror non package installer files #680

Conversation

sliverc commented Nov 27, 2017 • edited Loading

Description of the Change

Checklist

sliverc commented Nov 27, 2017

gittygoo commented Apr 13, 2018

codecov bot commented May 17, 2018 • edited Loading

Codecov Report

sliverc commented May 17, 2018

hsitter commented May 18, 2018

hsitter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smira commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sliverc commented May 22, 2018

smira left a comment

Choose a reason for hiding this comment

sliverc commented May 28, 2018

sliverc commented Jun 19, 2018

gittygoo commented Jun 22, 2018

sathieu commented Jan 22, 2019

sliverc commented Nov 27, 2017 •

edited

Loading

codecov bot commented May 17, 2018 •

edited

Loading