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

Ignore SYNC_PORT and SYNC_BASE in syncserver Dockerfile #3716

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

omarkohl
Copy link
Contributor

Note: I recommend reviewing only the last commit of this PR since the first one is a separate PR #3714 and the second one also is #3715 . I will rebase things as needed. (a.k.a. stacked diffs).

Hardcode them to:

SYNC_PORT=8080
SYNC_BASE=/anki_data

If these env variables are passed into the container with different values,
they are ignored.

The reasons is if the user modifies SYNC_BASE they risk data loss since
anki-sync-server will no longer write data into the volume. If they change
SYNC_PORT they need to also change it when mapping this internal port to the
external port of the container, which could be confusing plus it has no benefit
to allow this since it's always possible to change the external port even if
the internal port is fixed to 8080 (e.g. -p 1234:8080).

In both cases there is no benefit to making these values configurable and there
are risks associated.

Unfortunately there is no easy way of implementing this for the
Dockerfile.distroless so it's up to the user not to modify these values.

@dae
Copy link
Member

dae commented Jan 12, 2025

Seems reasonable. @jeankhawand?

@jeankhawand
Copy link
Contributor

jeankhawand commented Jan 18, 2025

@dae LGTM! Thanks, @omarkohl. I’m wondering about existing users migrating to this newer build—they will likely lose their existing decks. In this case, I believe Anki will check for diffs and display a prompt asking users whether to sync locally or fetch changes from AnkiWeb. Therefore, it’s recommended that users sync their local data to AnkiWeb.

@omarkohl
Copy link
Contributor Author

Thanks, @omarkohl. I’m wondering about existing users migrating to this newer build—they will likely lose their existing decks. In this case, I believe Anki will check for diffs and display a prompt asking users whether to sync locally or fetch changes from AnkiWeb. Therefore, it’s recommended that users sync their local data to AnkiWeb.

Before my commit b189820 from a little over a week ago no data was stored in a volume, so removing the container and starting a new one based on a new image would lead to data loss. That is why I created that commit, because otherwise data loss for users trying to upgrade their image was almost unavoidable (probably the only exception is if they manually created a volume for /home/anki/.syncserver). Upgrading the docker container was basically not possible., Now it is possible, even though upgrading from before b189820 to after it remains a little tricky. Or am I missing something?

Asking users to force sync from device to server is probably the easiest solution, yes. Are you suggesting to include that in the README or what were you trying to say?

@jeankhawand
Copy link
Contributor

jeankhawand commented Jan 19, 2025

Thanks, @omarkohl. I’m wondering about existing users migrating to this newer build—they will likely lose their existing decks. In this case, I believe Anki will check for diffs and display a prompt asking users whether to sync locally or fetch changes from AnkiWeb. Therefore, it’s recommended that users sync their local data to AnkiWeb.

Before my commit b189820 from a little over a week ago no data was stored in a volume, so removing the container and starting a new one based on a new image would lead to data loss. That is why I created that commit, because otherwise data loss for users trying to upgrade their image was almost unavoidable (probably the only exception is if they manually created a volume for /home/anki/.syncserver). Upgrading the docker container was basically not possible., Now it is possible, even though upgrading from before b189820 to after it remains a little tricky. Or am I missing something?

Asking users to force sync from device to server is probably the easiest solution, yes. Are you suggesting to include that in the README or what were you trying to say?

Could you please mention it in Readme so the users are aware about that and once they migrate to newer image they must sync their local. For Distroless it will be a bit tricky cause there is no tty session dunno if docker cp will work so they can copy data from default to /anki_data

@omarkohl
Copy link
Contributor Author

done

@dae
Copy link
Member

dae commented Jan 25, 2025

Thanks Omar, happy to merge this in once the conflicts are addressed.

Hardcode them to:

    SYNC_PORT=8080
    SYNC_BASE=/anki_data

If these env variables are passed into the container with different values,
they are ignored.

The reasons is if the user modifies SYNC_BASE they risk data loss since
anki-sync-server will no longer write data into the volume. If they change
SYNC_PORT they need to also change it when mapping this internal port to the
external port of the container, which could be confusing plus it has no benefit
to allow this since it's always possible to change the external port even if
the internal port is fixed to 8080 (e.g. `-p 1234:8080`).

In both cases there is no benefit to making these values configurable and there
are risks associated.

Unfortunately there is no easy way of implementing this for the
Dockerfile.distroless so it's up to the user not to modify these values.
@dae dae merged commit eaec53b into ankitects:main Jan 25, 2025
1 check passed
@omarkohl omarkohl deleted the push-wkvturlknqlp branch January 25, 2025 14:25
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.

3 participants