Provide fallback for GovukAdminTemplate.environment_label #2319
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
It turns out that the GOVUK_ENVIRONMENT_NAME env var is not defined and
the Rails env is not development or test during the GH action job which
builds the image for deployment. This means that GovukEnvironment.name
is nil and we see the following exception [1]:
NoMethodError: undefined method `titleize' for nil:NilClass
I considered setting GOVUK_ENVIRONMENT_NAME in the GH action, but there
didn't seem to be any precendent for doing that.
I also considered providing a fallback within the implementation of
GovukEnvironment.name. However, I thought that might have other
undesirable consequences.
So I've implemented the fallback in
config/initializers/govuk_admin_template.rb which is a bit ugly, but
hopefully it won't be too long before we get rid of GovukAdminTemplate
altogether!
[1]: https://github.com/alphagov/signon/actions/runs/5940071624/job/16107859623#step:7:609
chrisroos
approved these changes
Aug 22, 2023
There was a problem hiding this comment.
@floehopper and I have just discussed this. Although we think there might be a slightly nicer solution we've agreed to go with this for now so that we can unblock deployments.
floehopper
added a commit
that referenced
this pull request
Aug 23, 2023
In #2319 we fixed an issue which was triggered because this env var was not defined in the build-and-publish-image job in the deploy GH action. This job uses the shared-build-and-push-image job [1] which builds a Docker image [2] using Signon's Dockerfile. This Dockerfile is based on [3] the alphagov/govuk-ruby-base image which sets the RAILS_ENV env var to "production" [4]. The Dockerfile also includes a command [5] to run the assets:precompile task. At this point the Rails app boots up and the code in config/initializers/govuk_admin_template.rb is executed. Before the fix this resulted in a call to String#titleize on GovukEnvironment.name [6] which was nil, due to the combination of RAILS_ENV being set to "production" and GOVUK_ENVIRONMENT_NAME not being set, which raised a NoMethodError exception. While #2319 fixed the specific scenario described above, we realised that there might be other processes where RAILS_ENV is set to "production" and GOVUK_ENVIRONMENT_NAME is not set. So it's conceivable that there are scenarios where GovukEnvironment.name could be nil and the app might misbheave without raising an exception (e.g. in generating email copy) which is actually what was happening before the fix in #2311. It seems better to find out about such issues as soon as possible, so this commit reverts the changes in config/initializers/govuk_admin_template.rb and changes GovukEnvironment.name to fail fast if GOVUK_ENVIRONMENT_NAME is not defined. This also has the advantage that if the env var is missing, the exception & stack trace should make it a lot clearer what the problem is. With this change in place, in order not to fall foul of the problem we fixed in #2319, we need to set the GOVUK_ENVIRONMENT_NAME env var in the Dockerfile which is used in the build-and-publish-image job mentioned above. The actual value of GOVUK_ENVIRONMENT_NAME is not important here; it's just important that it is set to something so the Rails app can boot up successfully and be used to precompile the assets. The value of GovukAdminTemplate.environment_label which is set in config/initializers/govuk_admin_template.rb is only used to set the text of the main heading in the govuk_admin_template layout which is irrelevant in the precompiling of assets. So we've followed the pattern established by DEVISE_PEPPER & DEVISE_SECRET_KEY [7] and set GOVUK_ENVIRONMENT_NAME to "unused". Note that I've chosen to use ClimateControl to set the value of GOVUK_ENVIRONMENT_NAME in tests, because it provides a more realistic ENV in the test and makes the test less implementation-specific. We should probably be using ClimateControl in all tests instead of stubbing methods on ENV as per these recommendations [8], but I plan to tackle that separately. [1]: https://github.com/alphagov/govuk-infrastructure/blob/6f63a5722279252a8a24e66a09607e7900ffe0c0/.github/workflows/build-and-push-image.yml#L54 [2]: https://github.com/alphagov/govuk-infrastructure/blob/6f63a5722279252a8a24e66a09607e7900ffe0c0/.github/workflows/build-and-push-image.yml#L140 [3]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L2 [4]: https://github.com/alphagov/govuk-ruby-images/blob/f15874ab79fbdaa53cd5d4dd2fd92086efe0d5a6/base.Dockerfile#L99 [5]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L16 [6]: https://github.com/alphagov/signon/blob/9bc8214b1b34297fd344300e97c48570fe1cc6b7/config/initializers/govuk_admin_template.rb#L11 [7]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L8-L9 [8]: https://docs.publishing.service.gov.uk/manual/conventions-for-rails-applications.html#testing-utilities
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.
It turns out that the
GOVUK_ENVIRONMENT_NAMEenv var is not defined and the Rails env is not development or test during the GH action job which builds the image for deployment. This means thatGovukEnvironment.nameisniland we see the following exception:I considered setting
GOVUK_ENVIRONMENT_NAMEin the GH action, but there didn't seem to be any precendent for doing that.I also considered providing a fallback within the implementation of
GovukEnvironment.name. However, I thought that might have other undesirable consequences.So I've implemented the fallback in
config/initializers/govuk_admin_template.rbwhich is a bit ugly, but hopefully it won't be too long before we get rid ofGovukAdminTemplatealtogether!