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

Transmission home mountpoint set to /config #1974

Merged
merged 1 commit into from
Oct 16, 2021

Conversation

pkishino
Copy link
Collaborator

@pkishino pkishino commented Sep 29, 2021

Breaking change

This changes default mountpoint for transmission-home to /config in line with proper configurations.
if other mount point is set in env variables, a warning will be logged (it will still work but not recommended)

Proposed change

Change default mount for transmission-home to /config, see issue linked

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to a this container)
  • Breaking change (fix/feature causing existing functionality to break)

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated

@pkishino pkishino requested a review from haugene September 29, 2021 01:29
@pkishino pkishino changed the base branch from master to dev September 29, 2021 01:30
@pkishino pkishino force-pushed the transmission-home-mountpoint branch from 5590024 to 42da954 Compare September 29, 2021 01:31
@pkishino pkishino changed the title Transmission home mountpoint Transmission home mountpoint set to /config Sep 29, 2021
@pkishino pkishino force-pushed the transmission-home-mountpoint branch from 42da954 to 80d2ca4 Compare October 1, 2021 02:15
@pkishino pkishino merged commit 317a0a7 into dev Oct 16, 2021
@pkishino pkishino deleted the transmission-home-mountpoint branch October 16, 2021 03:34
TRANSMISSION_HOME_SUBNAME=${TRANSMISSION_HOME##*/}
echo "Attempting to migrate old TRANSMISSION_HOME from /data/$TRANSMISSION_HOME_SUBNAME to /config/$TRANSMISSION_HOME_SUBNAME "
if [ -d "/data/$TRANSMISSION_HOME_SUBNAME" ] && [ ! -d "/config/$TRANSMISSION_HOME_SUBNAME" ]; then
mv "/data/$TRANSMISSION_HOME_SUBNAME" "/config/$TRANSMISSION_HOME_SUBNAME"
Copy link
Owner

Choose a reason for hiding this comment

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

I think many people have not mounted /config. So doing a mv will possibly move the home folder to a non-mounted file system within the container and it would be gone when the container is re-created.

I know that's not 100% true because we specify VOLUME /config in the Dockerfile, but... For a new Docker-user it's probably tricky to re-locate their transmission-home inside a volume.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now, if user has not set TRANSMISSION_HOME_MIGRATE then only the warning is shown..
While it makes more sense to keep it under /config… I’m not against a user saying they don’t want that etc..
this would mainly affect new installs and optionally the user can let the system migrate it for them..
Perhaps add another check to see if the user has mounted /config or not?

Copy link
Owner

@haugene haugene Oct 18, 2021

Choose a reason for hiding this comment

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

Ah, sorry. I was a bit quick there, hadn't read it properly it seems 🙈
EDIT: And then I did it again, not reading all the code. I'm signing off for now, but the gist of what I was trying to communicate is...

If we change the default TRANSMISSION_HOME value in our Dockerfile I think most users will come to a blank Transmission state after they re-create the image next time (it will still be in /data though). The NAS gang usually has TRANSMISSION_HOME set because they get all the env vars in their GUI, but I would expect most of the rest to not have the env var set and just rely on the default.

If I'm not mistaken about the fallout here, then that's a bit too much breakage IMO. So I think we have to look for /data/transmission-home and then use that if it exists and log that they are using a non-default setup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes, I ate and I’ll fix a fallback after the little one has been dropped off at dagis ;)

@haugene
Copy link
Owner

haugene commented Oct 18, 2021

I agree that this data should live under /config. But we need to make the transition a bit smoother I think. Ref my comment on the userSetup.sh script. Could we symlink? Could we check if /config is mounted?

I'm more comfortable with a hard error telling the user to mount /config than moving user data without the user knowing.
We could link them to a page (gist or anything) saying "stop the container", "do docker cp .. ..", mount this/that and start again.
But I think this will lead to a lot of noise for a while 🙈

Should we do 5.0 release and then just hard-error telling people to revert to 4.x or fix their mounts? I'm not sure here.

@pkishino
Copy link
Collaborator Author

We wouldn’t be moving anything without the user explicitly saying so.. and I’m not sure I agree on the hard stop. In the end it should be up to the user how they want to set it up etc, but our recommended setup has a mount for /config in user land moving forward, as it makes more sense to keep config there.. but I don’t see any problems or such of the user wants to keep the current setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants