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

Update syno-plex-update.sh #2

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Conversation

programatix
Copy link
Contributor

  • Updated PLEX_PREFERENCES_FILE for DSM 7
  • Updated version comparison syntax

- Updated ```PLEX_PREFERENCES_FILE``` for DSM 7
- Updated version comparison syntax
Copy link
Owner

@YuriyGuts YuriyGuts left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! I have a couple of suggestions below

Comment on lines +19 to +20
if [ "${DSM_VERSION}" -ge "7" ]; then
PLEX_PREFERENCES_FILE='/var/packages/PlexMediaServer/shares/PlexMediaServer/AppData/Plex Media Server/Preferences.xml'
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! I wasn't aware of the symlinked path; this definitely looks cleaner than volume1

Comment on lines 19 to 26
PLEX_RELEASE_API='https://plex.tv/api/downloads/5.json?X-Plex-Token=TokenPlaceholder'
PLEX_RELEASE_API='https://plex.tv/api/downloads/5.json?channel=plexpass&X-Plex-Token=TokenPlaceholder'
Copy link
Owner

Choose a reason for hiding this comment

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

What is the benefit of using channel=plexpass here? I'd expect that not all users (including myself) use the paid services, so we should make sure free Plex users aren't locked out from using the package due to downloading an exclusive version. Alternatively, this could also be a config option to suit both types of users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for free users, X-Plex-Token is not required. Yes. it should be made into an option either by a config file or passed in as parameter.

Comment on lines 159 to 172
if [ ! -z "${latest_version}" ] && [ "${latest_version}" != "${installed_version}" ]; then
set +eu
/usr/bin/dpkg --compare-versions "$latest_version" gt "$installed_version"
if [ "$?" -eq "0" ]; then
set -eu
write_log 'Update available. Trying to download and install'
notify_update_available
download_url=$(parse_download_url "${release_meta}")
download_and_install_package "${download_url}"
else
set -eu
Copy link
Owner

Choose a reason for hiding this comment

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

Using dpkg comparison is a nice improvement 👍 This might look cleaner as a separate function that manages set (?)eu internally. Something like:

function is_latest_version_installed {
    # Check that the installed version is at least as high as the available version.
    local available_version=$1
    local installed_version=$2

    # dpkg version comparison uses exit codes so we'll tolerate errors temporarily.
    set +eu
    /usr/bin/dpkg --compare-versions "$available_version" gt "$installed_version"
    local result="$?"
    set -eu

    echo ${result}
}

Updated as suggested.
@YuriyGuts YuriyGuts merged commit e844a00 into YuriyGuts:master Nov 24, 2021
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.

2 participants