Redirect asset requests to asset host #3627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisroos
force-pushed
the
redirect-asset-requests-to-asset-host
branch
4 times, most recently
from
1c0847c
to
278e594
Compare
chrisroos
force-pushed
the
redirect-asset-requests-to-asset-host
branch
from
278e594
to
e9fde8d
Compare
chrisroos
changed the title
chrisroos
force-pushed
the
redirect-asset-requests-to-asset-host
branch
from
e9fde8d
to
21da32f
Compare
floehopper
approved these changes
Dec 20, 2017
| asset_filesystem_path = File.join(Whitehall.clean_uploads_root, 'asset.txt') | ||
| FileUtils.touch(asset_filesystem_path) | ||
|
|
||
| Plek.any_instance.stubs(:public_asset_host).returns('http://asset-host.com') |
There was a problem hiding this comment.
Minor: This line appears in both tests and could be extracted into a setup block.
There was a problem hiding this comment.
Good spot. I've moved this to a setup block.
chrisroos
added a commit
that referenced
this pull request
Dec 20, 2017
We want Whitehall assets to be served by the asset host (instead of either by the asset host or gov.uk) so that we can make caching and routing changes in a single place. PR #3627 adds a redirect to ensure that asset requests made to gov.uk are redirect to the asset host. This change will ensure that we're using the canonical asset host when writing consultation content to the Content Store. These URLs are rendered in the "Ways to respond" section on a consultation page.
10 tasks
chrisroos
force-pushed
the
redirect-asset-requests-to-asset-host
branch
from
21da32f
to
2f1278f
Compare
|
Thanks, @floehopper. I've addressed your comment and force pushed in preparation for merging. |
This `public_upload` route was unnecessarily complicated. Rails will treat anything after the final period in a wildcard route as the format[1] by default. By adding the `:extension` dynamic segment we were essentially duplicating that default behaviour. This meant that if we were to request a file containing more than one period, e.g. /government/uploads/path/to/file.name.png then `params[:path]` would contain 'path/to/file', `params[:extension]` would contain 'name' and `params[:format]` would contain 'png'. This caused me a problem when trying to redirect asset requests to the canonical asset host as calling `url_for(params)` (using the params above) would end up dropping the format of the file. I attempted to rename the `extension` segment in the other asset/attachment routes but struggled to rename it for the attachment preview route: In that case, the format is actually HTML even though the extension will (always?) be CSV. [1]: http://guides.rubyonrails.org/routing.html#route-globbing-and-wildcard-segments
As per the Route globbing and wildcard segments docs[1]: > If you want to make the format segment mandatory, so it cannot be omitted, you can supply format: true like this: We'd already made format mandatory by including the :format dynamic segment in the route. Using the `format: true` option appears to be the Rails way, though. [1]: http://guides.rubyonrails.org/routing.html#route-globbing-and-wildcard-segments
It's currently possible to request Whitehall assets (and attachments) at /government/uploads via both www.gov.uk and the asset host (e.g. assets.publishing.gov.uk). We want all asset requests to go via the asset host so that we can make changes to the config in a single place. I'm using `Plek#public_asset_host` as it returns "The public-facing asset base URL"[1]. I've tested this on integration and confirmed that it returns 'https://assets-origin.integration.publishing.service.gov.uk'. I initially tried to do the redirect in the routes file but it was proving to be quite hard so I've gone with a method in the `PublicUploadsController` instead. I had to update upload_access_test to simulate making requests against the asset host. Otherwise all the tests failed because they were being redirect to the asset host. Note that attachments are still available on both www.gov.uk and the asset host. I expect we'll update this when we start working on the migration of attachments to Asset Manager. [1]: https://github.com/alphagov/plek/blob/4d63a574ab36e9e8833ff14471482d1cbb110e61/lib/plek.rb#L97
This was made redundant in 616744b (Sep 2013). I noticed it when fixing some tests in this file in the previous commit.
chrisroos
force-pushed
the
redirect-asset-requests-to-asset-host
branch
from
2f1278f
to
cd37d37
Compare
chrisroos
added a commit
that referenced
this pull request
Dec 20, 2017
We want Whitehall assets to be served by the asset host (instead of either by the asset host or gov.uk) so that we can make caching and routing changes in a single place. PR #3627 adds a redirect to ensure that asset requests made to gov.uk are redirect to the asset host. This change will ensure that we're using the canonical asset host when writing consultation content to the Content Store. These URLs are rendered in the "Ways to respond" section on a consultation page.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch ensures that assets (but not attachments) are all served from the asset host (e.g. assets.publishing.service.gov.uk). This addresses alphagov/asset-manager#348 and will allow us to configure things like caching and routing in a single place.