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.

Sign up for GitHub

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

Jump to bottom

Added fallback and more explicit warning message #2020

Merged
merged 4 commits into from
Oct 21, 2021
Merged

Added fallback and more explicit warning message #2020

merged 4 commits into from
Oct 21, 2021
+30 −34

Conversation

pkishino
Copy link
Collaborator

Breaking change

Proposed change

This will use /data/transmission-home (previous default) as fallback if the folder exists.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue: relates to #
  • Link to documentation updated (if done separately): https://...

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

haugene
haugene approved these changes Oct 20, 2021
View reviewed changes
Copy link
Owner

@haugene haugene left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍 I just have one proposal for an additional log line. But I've been thinking a bit on how to make this migration as pain free, and user error free, as possible.

It might be a good help to offer the TRANSMISSION_HOME_MIGRATE option, but are we sure we need it? And will it cause more harm than good in some cases?

As the message reads right now (and assuming I'm an inexperienced user, aka noob) I might just add TRANSMISSION_HOME_MIGRATE=true and hope that solves whatever it is has to happen. If the user has not mounted /config before doing it then the data will get lost on the next container re-create.

Could v5 of the container just be all about setting the default TRANSMISSION_HOME to /config/transmission-home/ instead and do a fallback with a warning if /data/transmission-home/ exists? Or is that a too small step? It will make sure that new setups get off on the right foot, and then we can link to a separate readme page or something saying that they need to mount /config and copy that folder from /data. This way we know that they know what they're doing.

And we get away with less lines of code for later. If we introduce the migration option we'd have to support it for a while I guess?

transmission/userSetup.sh Show resolved Hide resolved
@pkishino
Copy link
Collaborator Author

Yeah, I understand. I’ll remove the migrate and just add a message instead

@pkishino pkishino merged commit 86eb6a8 into dev Oct 21, 2021
@pkishino pkishino deleted the config-relocate branch October 21, 2021 06:46
@ilike2burnthing
Copy link
Contributor

Have I misunderstood this? My log shows:

TRANSMISSION_HOME is set to: /config
WARNING: TRANSMISSION_HOME mountpoint is not on default /config, this is not recommended.

I have TRANSMISSION_HOME set to /config, the log shows this but in the next line says it's not.

@pkishino
Copy link
Collaborator Author

pkishino commented Oct 21, 2021 via email

@ilike2burnthing
Copy link
Contributor

Odd, it was on /config/transmission-home and I was getting that error, that's the only reason why I changed it. But that's when /data/transmission-home still existed and I was also getting the error WARNING: Deprecated. Found old default transmission-home folder in the /data mount, setting this as TRANSMISSION_HOME.

Changed it back to /config/transmission-home, moved the files, no error in the logs anymore.

@pkishino
Copy link
Collaborator Author

hmm,
sorry to ask this, but could you outline from 0 what you did?
I'm assuming
0: switched to :dev branch
1: modified TRANSMISSION_HOME variable?
..

@ilike2burnthing
Copy link
Contributor

You're fine, don't worry. I'm always running dev branch and use Watchtower for updates.

  • I noticed a torrent was stalled at 0%
  • removed and re-added it, didn't work
  • added a magnet, it couldn't pull the torrent information
  • I restarted the container, no luck
  • checked the logs and saw those two errors, stopped the container
  • changed the TRANSMISSION_HOME variable
  • copied the files from /data/transmission-home to /config
  • restarted the container, still had both errors, issue wasn't resolved
  • stopped the container again
  • deleted the /data/transmission-home folder
  • restarted the container, torrent immediately worked and only had the one error
  • posted about it here.

@pkishino
Copy link
Collaborator Author

pkishino commented Oct 22, 2021 via email

@ilike2burnthing
Copy link
Contributor

From /config/transmission-home to /config

@pkishino
Copy link
Collaborator Author

ah, yes, that would be a problem.. it should be /config/transmission-home at least.. just setting to /config would cause other issues..

@pkishino
Copy link
Collaborator Author

I hope I clarified the changes here to make it easier to understand:
53f9a38

@XMB5 XMB5 mentioned this pull request Nov 27, 2021
Merged
5 tasks
@ilike2burnthing ilike2burnthing mentioned this pull request Nov 29, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haugene haugene haugene approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pkishino @ilike2burnthing @haugene