GoogleContainerTools / rules_distroless

Apache License 2.0
54 stars 33 forks source link

Made package index support plaintext #80

Open antholeole opened 3 months ago

antholeole commented 3 months ago

This is useful for something like https://storage.googleapis.com/bazel-apt/, which (edit: used to!) only supports plaintext.

Change-Id: Ic3698c5e281b24a6017c034c164b657d1076a11e

jjmaestro commented 2 months ago

@antholeole nice! IMHO the more support for all indexes, the better :) Also, personally, I'd add tests to the PR so that there won't be regressions in the future 😃

antholeole commented 2 months ago

@jjmaestro added tests! ptal again

jjmaestro commented 2 months ago

I've re-read the commit and I don't think the test is exercising the code. The extensions in supported_extensions are evaluated in order and the bazel-apt repo does have other compressed indexes (you can try with e.g. https://storage.googleapis.com/bazel-apt/dists/testing/jdk1.8/binary-amd64/Packages.gz), so the for-loop in package_index will break before it gets to the plaintext download, etc. Maybe they've added the compressed indexes in the interim! :-/

Also, the test is IMHO a bit messy since it's adding the "exception" for Bazel on amd64.

So... if there's no single debian repo with only a plain text index, it's going to be hard to add a test that exercises the code. Sorry for my initial suggestion! 😅

I still think it's a valuable addition to support plaintext indexes since it's a valid index format per the Debian repository format spec so hopefully it'll be merged!

antholeole commented 2 months ago

Thats so strange. When i was writing the PR yesterday, those .gz packages did not exist despite their LastModified claiming it did... I'll leave the test up, since despite not actually testing no-extension indexes - it also tests a variety of other intestesting things, like using packages from two different repos as well as non-debian.org indexes.

regardless, applied your suggestion!

jjmaestro commented 2 months ago

@antholeole I still think it's not great to change the behavior of the e2e test depending on the architecture, especially when the test is not really exercising anything in the new code or anything special in the old code.

Using packages from different repos

Unless I'm mistaken, that's is already the default. Every channel is already its own repo and packages are pulled from all of the channels.

non-debian.org indexes

Well, either it follows the Debian specs or it will fail, regardless of the URL being a debian.org URL so... 🤷

If you really want to have something like this, you could create a new example in the examples folder, but again, consider that it's not really testing anything at all that's not already tested and/or shown in other examples.

In any case, I'm not a maintainer of the repo, so this is just my opinion!