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

Add GovukProxy::StaticProxy for development #261

Merged
merged 6 commits into from
Sep 5, 2022
Merged

Conversation

theseanything
Copy link
Contributor

@theseanything theseanything commented Sep 1, 2022

This class can be loaded as Rack middleware and will proxy any requests with the path prefix of /assets/static to backend specified. This is useful as it enables Static to use relative paths and the dependent frontend application can forward those requests. This allows us to switch Static to use relative path and allow us to have development environment and not run Static locally (i.e. forward request to production version of Static).

Rails can be made to initialise the proxy middleware by setting the GOVUK_PROXY_STATIC_ENABLED=true env var.

This is library allows us to add rack middleware to proxy requests.
govuk_app_config.gemspec Show resolved Hide resolved
spec/lib/govuk_proxy/static_proxy_spec.rb Outdated Show resolved Hide resolved
theseanything added a commit to alphagov/collections that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_URL env var to enable the proxy to Static in production. This is so the application continues, when Static changes to use relative URLs for assets. See alphagov/govuk_app_config#261 and alphagov/govuk-puppet#11801
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Really great idea here Sean - implementation looks nice and simple too which is great 👍

Only substantial comment is regarding usage of Plek.

@@ -1,5 +1,10 @@
module GovukAppConfig
class Railtie < Rails::Railtie
initializer "govuk_app_config.configure_govuk_proxy" do |app|
static_url = ENV["GOVUK_PROXY_STATIC_URL"]
app.middleware.use GovukProxy::StaticProxy, backend: static_url if static_url.present?
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about switching from the previous iteration where the env var acted as a toggle rather than defining the URL.

It looks a bit more painful doing this rather than using Plek since you'll always have to set two env vars to the same thing to use this. I'm not sure there is a scenario where someone would want a different url for this than they'd have for static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup agree - was a bit clunky having to set to URLs. Switched by to using Plek.

end
end

def test_request(path, proxied)
Copy link
Member

Choose a reason for hiding this comment

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

You should define this helper method within the RSpec describe method, as it'd be only scoped to tests there, whereas here it is a global function available to the whole app (defined as a method on Object)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

let(:app_domain) { "app.domain" }
let(:static_domain) { "static.domain" }

subject { GovukProxy::StaticProxy.new(app, backend: "https://static.domain", streaming: false) }
Copy link
Member

Choose a reason for hiding this comment

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

I think this use of unnamed subject would fall foul of RSpec linting rules. I've had a look and they're not on for this app so predictably running them is a bit grim. May want to just change this to

let(:proxy) { described_class.new(app, backend: "https://static.domain", streaming: false) } to not leave linting issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - thanks!

This class can be loaded as Rack middleware and will proxy any requests
with the path prefix of `/assets/static` to backend specified. This is
useful as it enables Static to use relative paths and the dependent
frontend application can forward those requests. This allows us to
switch Static to use relative path and allow us to have development
  environment and not run Static locally (i.e. forward request to
  production version of Static).
This is library allows us to resolve the URL for Static.
This adds a railtie to initialise the proxy middleware with backend set
to the URL resolved by Plek for Static. This allows frontend apps to
enable the proxying behavior in development by simply setting the env
var GOVUK_PROXY_STATIC_ENABLED to "true".
theseanything added a commit to alphagov/collections that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/finder-frontend that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/frontend that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/government-frontend that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/info-frontend that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/licence-finder that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/service-manual-frontend that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/smart-answers that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
theseanything added a commit to alphagov/govuk-docker that referenced this pull request Sep 1, 2022
This sets the GOVUK_PROXY_STATIC_ENABLED env var to enable the proxy to
Static in production. This is so the application continues, when Static
changes to use relative URLs for assets. See
alphagov/govuk_app_config#261 and
alphagov/govuk-puppet#11801
@theseanything theseanything merged commit 08fd9cf into main Sep 5, 2022
@theseanything theseanything deleted the add-govuk-proxy branch September 5, 2022 09:22
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.

3 participants