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.

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

Addon files #473

Closed
wants to merge 20 commits into from
Closed

Addon files #473

wants to merge 20 commits into from

Conversation

btkostner
Copy link
Contributor

@btkostner btkostner commented Jan 12, 2017

Fixes #384

Description of the Change

This is a follow up / redo of my #420 appstream PR. I realized I made the previous PR too narrow, and it touched a lot of code that it did not need to. Here is what this does:

  • Adds an addon folder at .aptly/addon that can store files you want to be in the repository
  • On publish, aptly will look in that folder and write the files to the repository, and release files
  • I did not add any API or CLI tags to change the addon directory, as that would be bad security
  • All files will be copied AS IS. No compressing or signing

The addon folder mirrors the public folder, so on publish .aptly/addon/dists/maverick/main/dep11/Components-i386.yml.gz will create .aptly/public/dists/maverick/main/dep11/Components-i386.yml.gz or equivalent.

Checklist

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

@btkostner
Copy link
Contributor Author

btkostner commented Jan 12, 2017

I have a couple of questions before this is all complete about the checklist:

  • All of the functions will be tested with functional tests. Do you still want unit tests for the trivial golang functions? Most of it's file system work, which would be hard to test without some weird mocking.
  • I wrote a basic test in python, but does not seem to work. I'm guessing because the file is getting removed before the test. Any chance I could get some help on this one?
  • Since I didn't add any CLI or API tags, is there anything you want me to add to the man pages?

Thanks you!

@smira
Copy link
Contributor

smira commented Jan 24, 2017

All of the functions will be tested with functional tests. Do you still want unit tests for the trivial golang functions? Most of it's file system work, which would be hard to test without some weird mocking.

I don't think we need unit-tests for things covered with functional testing. I prefer functional tests, but some things are hard to test (e.g. retry algorithm). So if covered with functional, we don't need any unit-tests.

@btkostner
Copy link
Contributor Author

btkostner commented Feb 3, 2017

I added a functional test that:

  • creates a simple test file
  • ensure it's published when aptly publishes a repository
  • ensure the checksum is in release file

Let me know if there is anything else for this you want covered.

@janisppc
Copy link

As this is still open are there any future plans for this AppStream (addon files) feature?

@smira
Copy link
Contributor

smira commented Feb 8, 2018

@btkostner @janisppc I'll take a look tomorrow

@btkostner
Copy link
Contributor Author

This should be up to date, and ready to merge now

Copy link
Contributor

@hsitter hsitter left a comment

Choose a reason for hiding this comment

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

Code LGTM

Probably also needs some documentation on https://github.com/aptly-dev/aptly-dev.github.io (as standalone feature page?), explaining how a user might want to use that and what the directory structure needs to look like.

deb/publish.go Outdated
}

fsPath := filepath.Join(addonDir, p.Prefix, "dists", p.Distribution, component)
if err := filepath.Walk(fsPath, func(path string, info os.FileInfo, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use Walk here I think it also needs to implement some level of symlink handling. I can entirely imagine people wanting to keep the data in some other dir and then ln -sf resources it into the addon dir as they get updated.

@hsitter
Copy link
Contributor

hsitter commented Apr 30, 2018

@btkostner If you could rebase with master that'd be lovely, and maybe have a look at the symlink walking and documentation.

We are also having some concerns over the name. Addon makes it sound like it's some sort of addon system for aptly in general. @sliverc suggested skel as a possible name, which I find fairly reasonable. Alternatively I'd offer up PublicAddonData.

I'd love to get this merged soon ™️

@hsitter hsitter added the 1.4.0 label Apr 30, 2018
@btkostner
Copy link
Contributor Author

Test failure is unrelated to this PR

@btkostner
Copy link
Contributor Author

@apachelogger Updated and renamed to skel in the code and skeleton files in some of the docs.

As far as the symlink walking, that is going to be a lot of extra work. As far as I know (but it's been a long time since I've touched go so I could be wrong) there is no standard way of handling symlinks.

@btkostner
Copy link
Contributor Author

Opened up a basic doc PR. It might need more information.

@hsitter
Copy link
Contributor

hsitter commented May 2, 2018

The system tests will make me go insane some day -.-

@btkostner Thanks. You are absolutely right about the symlink, I misremembered how Walk works. We'd have to manually readdir and descend into symlinks, so let's just ignore this until someone actually complains.

I'll see what can be done about the failing system test.

@hsitter
Copy link
Contributor

hsitter commented May 23, 2018

I've had another look and I think this currently isn't actually up to expectations:

https://wiki.debian.org/DebianRepository/Format#MD5Sum.2C_SHA1.2C_SHA256

The checksum and sizes shall match the actual existing files. If indexes are compressed, checksum data must be provided for uncompressed files as well, even if not present on the server.

i.e. we need to temporarily uncompress and inject the checksums of the compressed files as well, even though we do not publish them (which we ought not if they were not in the skel)

You'll see this particularly affecting dep11 data created by appstream-generator. The latter only creates .xz and .gz and expects the archive software to properly create the checksums for the Release file.
Without the uncompressed entry apt will outright refuse to acknowledge the existance of the files (at least on ubuntu16.04, where I tested) and not even bother doing a GET request.

@rhertzog
Copy link

rhertzog commented Jul 8, 2019

@btkostner What't the status of this feature on your side? It would be really nice if we could have this working in aptly. Missing appstream metadata is a problem right now for us. I might be able to help with some sponsorship to get this completed.

@btkostner
Copy link
Contributor Author

@rhertzog Sadly I have no time to work on this. If I remember correctly, the only thing that needs to be fixed / added is uncompressed checksums not being generated.

@lbolla lbolla closed this Jan 28, 2022
@silopolis
Copy link

This would be a really nice addition and probably make Aptly stand out of the crowd, if not already ;)
@rhertzog I'd modestly but gladly tip this :)

@randombenj randombenj reopened this Jul 9, 2022
@silopolis
Copy link

silopolis commented Jul 10, 2022 via email

@rhertzog
Copy link

This would be a really nice addition and probably make Aptly stand out of the crowd, if not already ;) @rhertzog I'd modestly but gladly tip this :)

The customer that was investigating aptly at that time did not switch and as such I can't offer direct sponsorship but since this a Debian related project, you could possibly consider requesting a grant via https://salsa.debian.org/freexian-team/project-funding

@jhonny-oliveira
Copy link

Is there any chance this ever gets finalized and merged?
I don't mind testing it.

Thank you!

@silopolis
Copy link

@btkostner @rhertzog any estimate of the work needed and potential funding needed to get this merged?

@btkostner
Copy link
Contributor Author

I'm not going to lie, it's been half a decade since I've looked at this code. Considering how long this has been opened, it's probably going to be easier to use some of this code as reference and open a whole new PR

@randombenj
Copy link
Contributor

@reglim could you have a look at this? Lets discuss this next week :)

@jhonny-oliveira
Copy link

I would like to add that Gnome Software and Ubuntu Software Center require dep11 metadata to display the software, otherwise it won't display it. In addition, I did not manage to find other repository management tool that automatically generates/incorporates this data. So, having this functionality working on aptly would be really welcome.

If I got this correctly (correct me, if I'm wrong), the intention of this pull request was to merge pre-created dep11 data from the addon folder during the publish stage. Conceptually the idea seems quite straight forward and has the advantage of offloading the responsibility of creating the dep11 metadata away from aptly. Hopefully this also makes it easier to implement.

As I mentioned earlier, I don't mind testing it, documenting and making the new version available through XtraDeb.net for early adopters.

Cheers!

@reglim
Copy link
Contributor

reglim commented May 31, 2023

I just migrated your changes from btkostner:addon-files to the branch feature/384-generate-checksums-for-component-files. It's now up to date with the current master.

@randombenj
Copy link
Contributor

Let's close this in favour of: #1186

@randombenj randombenj closed this May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generate checksums for all Components files [$15]
10 participants