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

ci: Provide geonames database as GitHub release asset #10916

Closed
wants to merge 1 commit into from

Conversation

jvanbruegge
Copy link

As discussed on Discord, this adds an archive containing the geonames database at the time of release as release asset.
This is useful for reproducible builds such as on NixOS.

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

Strictly speaking this won't necessarily be the same version of the data as is in our release image. If that matters to you, an alternative option is to load up that image here and copy the data out of it (not as big of an issue here as it is for the nix build, since here the image will be in a very nearby cache).

@jvanbruegge
Copy link
Author

It does not quite matter if it is the same version as in the weekly container build, more that there is one fixed version per release

@zackpollard
Copy link
Contributor

zackpollard commented Jul 6, 2024

I didn't realise you wanted to do this in this repo, I don't like the idea of duplicating this script over here really, we now have to maintain this in two places

@jvanbruegge
Copy link
Author

jvanbruegge commented Jul 6, 2024

I could make it a script in the base-image repo and download that in this workflow instead

@jvanbruegge
Copy link
Author

jvanbruegge commented Jul 6, 2024

I fixed that now. The script that downloads the geodata is in this PR: immich-app/base-images#79
Here, the CI just downloads and executes that scripts and zips the folder. No more code duplication.

Note that this workflow will only work once the other PR is merged

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

This isn't even right though. This is probably not what is actually included in the release.

@zackpollard
Copy link
Contributor

This isn't even right though. This is probably not what is actually included in the release.

Yea, I think I have similar feelings, if we are going to publish these in the release they should be the actual ones we bundle in the release.

@jrasm91
Copy link
Contributor

jrasm91 commented Jul 6, 2024

If the goal is to just have a reproducible download, they can be added to git in a separate repository outside of immich. If we're going to attach them to the release, I would recommend just adding them to the base image release itself.

@jvanbruegge jvanbruegge force-pushed the geonames-release branch 3 times, most recently from 599e24b to 02bed34 Compare July 6, 2024 18:55
@jvanbruegge
Copy link
Author

jvanbruegge commented Jul 6, 2024

ok, now this is indeed saving the exact version that is also in the release docker container.
I tested it in my fork and it works: https://github.com/jvanbruegge/immich/actions/runs/9821421416/job/27117067837
https://github.com/jvanbruegge/immich/releases/tag/v1.108.0

@zackpollard
Copy link
Contributor

Will take a look at this on Monday and merge it in if all looks good! 🙂

@zackpollard zackpollard self-assigned this Jul 6, 2024
@jvanbruegge
Copy link
Author

Is there anything still to do here for me?

@bo0tzz bo0tzz requested a review from zackpollard July 15, 2024 07:57
@zackpollard
Copy link
Contributor

Hey, apologies for the silence on this. I'll give you as brief of an update as I can for now.

Essentially there are two issues here.
1.) We don't really want to have this linking between the repos that this introduces, we would prefer that this artifact is included as part of the base-image releases, though we don't currently create github releases for these.
2.) We are currently looking into overhauling the reverse geocoding and providing the option of a system with much improved accuracy. This will require a bunch more files to be created and stored, so it may completely overhaul the way we currently include the geonames data too, as the systems are related.

Basically, if we were to merge something like this, it would need to exist entirely within the base-images solution, but we are hesitant on that too right now due to the fact this may all change and potentially live in a separate repository pretty soon anyway, so don't want to add something that may immediately be changed. If that happens there will likely be automated releases for all this stuff included within that repo anyway.

@jvanbruegge
Copy link
Author

Ok, for the nix build I now used the internet archive as a stopgap solution until your new system is in place

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.

4 participants