spotDL / spotify-downloader

Download your Spotify playlists and songs along with album art and metadata (from YouTube if a match is found).
https://spotdl.readthedocs.io/en/latest/
MIT License
17.4k stars 1.6k forks source link

As a User, when in Docker, I would like the ability for downloads to head straight to the mounted '/music' location in the 'config.output' format (issue morphed into versioned and nightly dev builds PR) #1724

Closed othyn closed 1 year ago

othyn commented 1 year ago

Requested Feature

Firstly, love the project, been using the CLI version for a long time.

In its current form, the Docker application serves as a GUI to interact with the spotDL binary, instead of having to use the CLI, correct?

If this is the intention, I would like the ability when using the Docker version to utilise it more as a hosted app that can place files into its mount point (/music) using the config.output format automatically, as in its current state a download option is presented to download the MP3 file once the download from YouTube + meta merge from Spotfiy is complete.

Admitedly, when using the web version of the application for the first time, this confused me, as I was expecting it to head into the mount point (/music) at the given config.output format.

My use case being something like youtubedl, where it just downloads files to the mounted location automatically. This way I can point the mount point for the Docker Compose file at my Plex music directory, so the spotDL service can download files straight into the managed Plex music directory.

Possible implementation

I have no problem submitting a PR for this, but some guidance on implementation would be appreciated.

I think it could possibly be achieved through the following:

  1. A new boolean config key, docker_web_dl_to_mount or something like that which simply stores whether the user, when in a Docker environment, would like the web tool to download the files to the mount location in the given expected config.output file structure/format.

  2. I imagine modifying:

https://github.com/spotDL/spotify-downloader/blob/23d18c33d484a21eb9781d13ca908c79fa2cbe9f/spotdl/utils/web.py#L273

So that if the docker_web_dl_to_mount config option is true, this line is either ignored, or mirrors:

https://github.com/spotDL/spotify-downloader/blob/23d18c33d484a21eb9781d13ca908c79fa2cbe9f/spotdl/download/downloader.py#L370

I'm working on the basis here that if state.downloader.output is not overriden, it will default to the output of create_file_name(song, self.output, self.output_format) as defined in download/downloader.py. In which case there are two solutions:

If state.downloader.output is purely an override of the default create_file_name call, then a simple wrap of the setting of state.downloader.output conditionally based on the value of docker_web_dl_to_mount will suffice:

# Not detailed: docker_web_dl_to_mount will be pulled in from config
if not docker_web_dl_to_mount:
  state.downloader.output = str(
      (get_spotdl_path() / f"web/sessions/{client_id}").absolute()
  )

Otherwise, something more verbose will be required:

# Not detailed: docker_web_dl_to_mount will be pulled in from config
# Will require a bit of refactoring, as the song isn't pulled until line 287
# Will also need to source output and output_format from config
if docker_web_dl_to_mount:
  output_file = create_file_name(song, output, output_format)
else:
  state.downloader.output = str(
      (get_spotdl_path() / f"web/sessions/{client_id}").absolute()
  )
  1. If the docker_web_dl_to_mount config option is true, the download button on the main web page needs to be disabled. Then just a small bit of validation on the download endpoint at https://github.com/spotDL/spotify-downloader/blob/23d18c33d484a21eb9781d13ca908c79fa2cbe9f/spotdl/utils/web.py#L313 to ensure that if docker_web_dl_to_mount is true, that endpoint just returns a 403.
xnetcat commented 1 year ago

Hi this feature will be available in the next version but you can already try it on the dev version. To use this use the --web-use-output-dir option or change it in the config file.

othyn commented 1 year ago

Oh sweet! Amazing, will give that a go!

othyn commented 1 year ago

Doesn't appear to be any easy way to create a Docker container based from the dev branch, without forking the repo, pulling in the dev branch an manually injecting it into the image.

Is there a rough ETA on the next release?

xnetcat commented 1 year ago

Doesn't appear to be any easy way to create a Docker container based from the dev branch, without forking the repo, pulling in the dev branch an manually injecting it into the image.

Is there a rough ETA on the next release?

Early February, the only things left to do are web ui (album/playlist/artist support, new settings) and tests (there is a lot of new features that don't have tests)

othyn commented 1 year ago

Awesome, shall wait patiently!

Or, I can submit a PR that adds support for extended tagging/nightly builds on your GitHub Actions scripts for the Docker Hub image builds, if that is something you guys are interested in?

xnetcat commented 1 year ago

Or, I can submit a PR that adds support for extended tagging/nightly builds on your GitHub Actions scripts for the Docker Hub image builds, if that is something you guys are interested in?

Sure, that would be awesome!

othyn commented 1 year ago

Cool, I'll take a look and submit a PR after work

othyn commented 1 year ago

Submitted a PR for nightly and versioned builds, with label and tag enhancements in #1725

xnetcat commented 1 year ago

Submitted a PR for nightly and versioned builds, with label and tag enhancements in #1725

Hmm looks like the job isn't running. Maybe it has to be on the master branch

othyn commented 1 year ago

Hmm looks like the job isn't running. Maybe it has to be on the master branch

Yeah GitHub Actions will only update their underlying workflow from the main branch of the repository, in this case its master. So none of the changes will take effect until its merged into master unfortunately.

xnetcat commented 1 year ago

Hmm looks like the job isn't running. Maybe it has to be on the master branch

Yeah GitHub Actions will only update their underlying workflow from the main branch of the repository, in this case its master. So none of the changes will take effect until its merged into master unfortunately.

Yeah that's what I've thought. I've already moved the commit to master branch. Running workflow now to see if everything is working https://github.com/spotDL/spotify-downloader/actions/runs/4019423049/jobs/6906243734

othyn commented 1 year ago

👀🤞

The generated image version and tags look good and its picked up the correct release event in which its also set the correct ref, all looks good so far, just have to wait for the arm64 build - always painfully long on QEMU hahah

othyn commented 1 year ago

Whoop whoop! We have success! 🥳 Although, I've just thought. The dev builds won't want the latest tag, as they're not production... let me come up with fix, two mins

Screenshot 2023-01-26 at 22 00 11
othyn commented 1 year ago

It will mean though that you'll need to fire off another master build to get that latest tag from being a nightly build, sorry about that

othyn commented 1 year ago

Been monitoring the builds, they appear to be going okay for now:

https://github.com/spotDL/spotify-downloader/actions/runs/4019840367 https://github.com/spotDL/spotify-downloader/actions/runs/4019890099

With the correct, and only, nightly tag being applied to dev builds (whew):

https://hub.docker.com/r/spotdl/spotify-downloader/tags

Although a new build will need to be actioned (checking the new 'Production build instead of nightly?' check box when manually firing the workflow) to get the latest tag back onto and aligned with master (at v4.0.7). The tag for 4.0.7 won't be picked up though, as that requires a tag/release event as the workflow trigger in order to have the correct ref assigned, which contains the tag in which the docker/metadata-action step can parse as a semver tag.

If you have any more issues or want anything changed, I'll pick it up and submit another PR in the morning 👍

Just a note, as I'm not sure if you are aware (if you are then please ignore!), I did add a concurrency check to the workflow:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

This just ensures that overlapped builds that would produce the same output, given they've come from the same workflow and ref, don't run. Only the latest run matching that criteria will run, all older still running actions with the same group will be killed.

This is mainly a thing to save Actions minutes but can catch you out if, for example, you run two builds at once from the same trigger.

othyn commented 1 year ago

Looks like we has success overnight! The manual latest build worked fine and rolled latest back to master:

https://github.com/spotDL/spotify-downloader/actions/runs/4020137836/jobs/6907730296

And the nightly build correctly went off at midnight on dev:

https://github.com/spotDL/spotify-downloader/actions/runs/4020257651/jobs/6907972502

🥳 Thanks for the merge, that will come in super handy!

xnetcat commented 1 year ago

Looks like we has success overnight! The manual latest build worked fine and rolled latest back to master:

https://github.com/spotDL/spotify-downloader/actions/runs/4020137836/jobs/6907730296

And the nightly build correctly went off at midnight on dev:

https://github.com/spotDL/spotify-downloader/actions/runs/4020257651/jobs/6907972502

🥳 Thanks for the merge, that will come in super handy!

Thanks for the PR 🚀, if you have any other feature suggestions let me know!