Setup routing for image and audio assets based on environment variables #107

Merged

eta357 merged 4 commits from 35-make-asset-paths-configurable into main 2024-10-16 17:48:14 +00:00

Conversation 5 Commits 4 Files Changed 5 +41 -4

clbertolini commented 2024-10-08 20:43:43 +00:00

Member

Copy Link

No description provided.

clbertolini added 1 commit 2024-10-08 20:43:43 +00:00

Setup routing for image and audio assets based on environment variables a43955726a

clbertolini was assigned by LibreTunes-Bot 2024-10-08 20:43:47 +00:00

LibreTunes-Bot commented 2024-10-08 20:43:47 +00:00

Member

Copy Link

This PR resolves #35.

This PR resolves #35.

clbertolini requested review from eta357 2024-10-08 20:43:59 +00:00

eta357 added the

backend

status

in review

labels 2024-10-08 20:44:25 +00:00

eta357 commented 2024-10-09 16:55:33 +00:00

Owner

Copy Link

@clbertolini Can you add these environment variables in example.env and Docker compose script?

@clbertolini Can you add these environment variables in `example.env` and Docker compose script?

eta357 commented 2024-10-10 17:11:33 +00:00

Owner

Copy Link

Also: Remove the TODO on line 52 in songdata.rs:

impl TryInto<Song> for SongData {
	type Error = Box<dyn std::error::Error>;

	/// Convert a SongData object into a Song object
	/// 
	/// The SongData/Song conversions are also not truly reversible,
	/// due to the way the image_path data is handled.
	fn try_into(self) -> Result<Song, Self::Error> {
		Ok(Song {
			id: Some(self.id),
			title: self.title,
			album_id: self.album.map(|album|
				album.id.ok_or("Album id must be present (Some) to convert to Song")).transpose()?,
			track: self.track,
			duration: self.duration,
			release_date: self.release_date,
			// TODO https://gitlab.mregirouard.com/libretunes/libretunes/-/issues/35
			storage_path: self.song_path,

			// Note that if the source of the image_path was the album, the image_path
			// will be set to the album's image_path instead of None
			image_path: if self.image_path == "/assets/images/placeholder.jpg" {
				None
			} else {
				Some(self.image_path)
			},
		})
	}
}

Also: Remove the TODO on line 52 in `songdata.rs`: ``` impl TryInto<Song> for SongData { type Error = Box<dyn std::error::Error>; /// Convert a SongData object into a Song object /// /// The SongData/Song conversions are also not truly reversible, /// due to the way the image_path data is handled. fn try_into(self) -> Result<Song, Self::Error> { Ok(Song { id: Some(self.id), title: self.title, album_id: self.album.map(|album| album.id.ok_or("Album id must be present (Some) to convert to Song")).transpose()?, track: self.track, duration: self.duration, release_date: self.release_date, // TODO https://gitlab.mregirouard.com/libretunes/libretunes/-/issues/35 storage_path: self.song_path, // Note that if the source of the image_path was the album, the image_path // will be set to the album's image_path instead of None image_path: if self.image_path == "/assets/images/placeholder.jpg" { None } else { Some(self.image_path) }, }) } } ```

clbertolini added 1 commit 2024-10-11 20:54:12 +00:00

Add example environment variables and update docker-compose b222b7911f

clbertolini added 1 commit 2024-10-11 21:17:02 +00:00

Remove TODO e90ea56d14

eta357 added 1 commit 2024-10-16 03:22:27 +00:00

Change audio and image path Docker comopse settings ... f39d93bd0c

Add volume for images

eta357 commented 2024-10-16 03:29:44 +00:00

Owner

Copy Link

Did some more investigation into this and I made a quick change, hope you don't mind. I initially misled you by telling you to put stuff in /site by default. This is all generated at compile-time, and includes the favicon and placeholder image. If we mount on top of it, those assets won't be accessible. If the first two rules (which you added) don't match a URI looking for some asset, the next route will look in the site directory and find those assets (favicon/placeholder).

The Docker compose environment variables reflect the default internal values of these variables, both in assets relative to the working directory, which in the Docker build is /.

Eventually (#116) the placeholder should be built directly in.

Did some more investigation into this and I made a quick change, hope you don't mind. I initially misled you by telling you to put stuff in /site by default. This is all generated at compile-time, and includes the favicon and placeholder image. If we mount on top of it, those assets won't be accessible. If the first two rules (which you added) don't match a URI looking for some asset, the next route will look in the `site` directory and find those assets (favicon/placeholder). The Docker compose environment variables reflect the default internal values of these variables, both in `assets` relative to the working directory, which in the Docker build is `/`. Eventually (#116) the placeholder should be built directly in.

eta357 approved these changes 2024-10-16 17:47:38 +00:00

eta357 merged commit 3cfcacc9ba into main 2024-10-16 17:48:14 +00:00

eta357 referenced this issue from a commit 2024-10-16 17:48:15 +00:00

Merge pull request 'Setup routing for image and audio assets based on environment variables' (#107) from 35-make-asset-paths-configurable into main

Sign in to join this conversation.

Reviewers

No Reviewers

eta357

Labels

Clear labels

backend

Pertaining to the API or back end of the web app

cicd

Relating to continuous integration / continuous deployment

dependencies

docs

Documentation work

frontend

Related to the UI or front end of the web app

meta

Related to the project itself

status

done

Work has been completed

status

in progress

Work is in progress

status

in review

Under review to be merged

status

todo

Work is planned, but has not yet started

type

bug

An issue with an existing feature

type

feature

A new feature

No Label

backend

status

in review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clbertolini dannyzou18 ecco257 eta357 hifwiend LibreTunes-Bot pubsafe

No Assignees clbertolini

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: LibreTunes/LibreTunes#107

No description provided.