Skip to content

support specifying asset host for local storage#185

Open
makkrnic wants to merge 1 commit intostavro:masterfrom
papagenocms:master
Open

support specifying asset host for local storage#185
makkrnic wants to merge 1 commit intostavro:masterfrom
papagenocms:master

Conversation

@makkrnic
Copy link

No description provided.

Signed-off-by: Mak Krnic <mak@makkrnic.com>
@yjukaku
Copy link

yjukaku commented Jul 18, 2017

👍 Would like to see this merged. It's hard to do testing of API assets urls when in dev.

@DmytroStepaniuk
Copy link

Still not merged... It would be awesome to have such feature!

config :arc,
storage: Arc.Storage.S3, # or Arc.Storage.Local
bucket: {:system, "AWS_S3_BUCKET"}, # if using Amazon S3
asset_host: "http://static.example.com" # or {:system "ASSET_HOST"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comma in example
{:system, "ASSET_HOST"}

@@ -16,10 +16,10 @@ defmodule Arc.Storage.Local do
def url(definition, version, file_and_scope, _options \\ []) do
Copy link

Choose a reason for hiding this comment

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

There's an optimization issue here, host() is called two times. Should be:

local_path = build_local_path(definition, version, file_and_scope)
host = host()

if host == "" do
  Path.join "/", local_path
else
  Path.join [host, local_path]
end

Path.join "/", local_path
else
"/" <> local_path
Path.join [host(), "/", local_path]
Copy link

Choose a reason for hiding this comment

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

This line breaks my tests as it creates URLs like "http://static.example.com/./arctest/uploads/original-binary%20file.png"

Should be Path.join [host, local_path]

@sebastialonso
Copy link

sebastialonso commented Jun 4, 2018

Did anything ever happened with this PR?

@Betree
Copy link

Betree commented Jun 4, 2018

@sebastialonso Nop. 4 months without merge on this repo. This is pretty annoying as this project hold a critical feature for web development.

@odarriba
Copy link

Any news on this @stavro ?

@hourliert
Copy link

It would be awesome to see that merged :)

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.

8 participants