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

Adding support for an AssetHost #795

Merged
merged 3 commits into from
Jun 7, 2019
Merged

Adding support for an AssetHost #795

merged 3 commits into from
Jun 7, 2019

Conversation

russ
Copy link
Contributor

@russ russ commented Jun 7, 2019

Added an asset_host option to the Lucky::Server.settings
That setting is prepended to the asset path. It can be configured in the
application the same way host and port are.

Purpose

Adding an asset_host option provides a reasonable way to configure production apps to use a cdn for assets.

fixes #794

Description

Lucky::Server.configure do |settings|
  if Lucky::Env.production?
    settings.asset_host = "production.com"
  else
    settings.asset_host = "/"
  end
end

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • [NA] - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

Added an asset_host option to the Lucky::Server.settings
That setting is prepended to the asset path. It can be configured in the
application the same way host and port are.
@jwoertink
Copy link
Member

Looks like the failure is related to ameba updating to only support crystal 0.29.0, and Lucky running on an earlier version.

@russ
Copy link
Contributor Author

russ commented Jun 7, 2019

Yeah, that is a versioning error. Should Lucky be locked to a specific version of Ameba or should this pr wait until Lucky is upgraded to 0.29.0?

@jwoertink
Copy link
Member

I think we need to lock all shards down to very specific versions. It's a bit more restrictive, but there's nothing worse than running shards update, and everything is borked due to some random update. (e.g. this...)

@paulcsmith
Copy link
Member

paulcsmith commented Jun 7, 2019 via email

Added a test for asset_path and dynamic_asset_path.
@russ
Copy link
Contributor Author

russ commented Jun 7, 2019

I added some specs around that. Still failing on the ameba version though.

@jwoertink
Copy link
Member

Just lock it to 0.9.1 here https://github.com/luckyframework/lucky/blob/master/shard.yml#L63. Then the next version we roll out, we'll update that to 0.10.0

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

I love it!

@paulcsmith paulcsmith merged commit c4db6b9 into luckyframework:master Jun 7, 2019
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.

Asset Host Option
3 participants