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

feat: support multiple URLs in sources #134

Merged

Conversation

jjmaestro
Copy link
Contributor

Fixes #113

The Debian snapshot mirror fails quite often and, while the Cloudflare mirror has had some issues in the past (e.g. it was lagging replication for months) it's much more reliable.

This commit adds support for multiple URLs in the manifest. See examples/debian_snapshot/bullseye.yaml for a sample usage.

@thesayyn
Copy link
Collaborator

You need a rebase here; so far looks okay to me.

@jjmaestro jjmaestro force-pushed the feat-support-multiple-src-urls branch 2 times, most recently from 767ab63 to 994b756 Compare January 21, 2025 20:07
@jjmaestro
Copy link
Contributor Author

This commit fixes the buildifier issues. I'll make the changes to the lock now.

@jjmaestro
Copy link
Contributor Author

@thesayyn thinking more about the lockfile, I was considering the "breaking change" and wondering if you would want / prefer the approach I did in #95: supporting v1 and a new v2, having auto-migration, etc.

If so, maybe you could consider (1) landing this PR as-is and (2) either checking the last comments I did on #93 so I could move that forward and then add the urls to #95... or I could also re-do #95 from scratch without depending on #93.

I'm just thinking that it would be nice to avoid the breaking change... let me know what you think!

@thesayyn
Copy link
Collaborator

Since we are pre 1.0 i'd be okay with just breaking things, that said maybe we could be less sensitive about url vs urls in the lockfile, accept both?

@jjmaestro
Copy link
Contributor Author

maybe we could be less sensitive about url vs urls in the lockfile, accept both?

true, true, sounds like a good compromise for now, I think! Same way the manifest can have both...

OK, I'll go about this and we can go over the other PRs later on (ah, that huge stack... I would definitely do things differently now XXD)

@jjmaestro jjmaestro force-pushed the feat-support-multiple-src-urls branch from 994b756 to d6b2868 Compare January 22, 2025 19:19
@jjmaestro
Copy link
Contributor Author

@thesayyn I've added urls to the lock, I've tested it manually with the original lockfiles and with an updated lockfile. Also, I've left all the lockfiles in "the old format" (with url), let me know if you want to update them all now or leave them as-is for later.

@jjmaestro jjmaestro force-pushed the feat-support-multiple-src-urls branch from d6b2868 to a4767ce Compare January 22, 2025 23:08
Copy link
Collaborator

@thesayyn thesayyn left a comment

Choose a reason for hiding this comment

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

LGTM, one small nit

Fixes GoogleContainerTools#113

The Debian snapshot mirror fails quite often and, while the Cloudflare
mirror has had some issues in the past (e.g. it was lagging replication
for months) it's much more reliable.

This commit adds support for multiple URLs in the manifest and the
lockfile. See examples/debian_snapshot/bullseye.yaml for a sample usage.
@jjmaestro jjmaestro force-pushed the feat-support-multiple-src-urls branch from a4767ce to 3a883cf Compare January 23, 2025 17:48
@thesayyn thesayyn merged commit 72692d8 into GoogleContainerTools:main Jan 23, 2025
10 checks passed
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.

FR: Support alternate URLs for package_index
2 participants