-
Notifications
You must be signed in to change notification settings - Fork 9
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
Allow bundling with local govuk_frontend_toolkit #1035
Conversation
startup.sh
Outdated
temporary_location="./tmp/govuk_frontend_toolkit_gem_dev" | ||
|
||
# Remove any existing tmp file | ||
rm -rf ${temporary_location} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels mildly dangerous
startup.sh
Outdated
rm -rf ${temporary_location} | ||
|
||
# Copy the old assets aside | ||
sudo cp -r ${installed_location} ${temporary_location} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is sudo
needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good for the comment to include why they are being copied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly because I didn't want to sudo rm -rf
I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the comment so it’s clear?
I ran this script against my local version of toolkit When I ran Gemfile.lock: - govuk_frontend_toolkit (6.0.1)
+ govuk_frontend_toolkit (6.0.3) Not sure if that might catch people out. Other than a couple of minor comments, seems good 👍 (Also needs a rebase) |
6eba63b
to
8dd9c20
Compare
@fofr updated the branch which addresses your out of date gem worry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge pending a rebase.
Just tried to run this and got the following error: Fixed by running |
This is a fix to the startup file which is currently failing. The output of "bundle show govuk_frontend_toolkit" currently contains warnings about us not running the latest version of bundler which means the script fails. This fix simply truncates the output at the first line.
…-toolkit-flag Fix output of finding installed location
|
||
if [[ $1 == "--test-govuk-frontend-toolkit" ]] ; then | ||
# Find out where it the gem is installed | ||
installed_location="$(bundle show govuk_frontend_toolkit | sed -1p)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line failed for me on the dev VM for a couple of reasons.
-
sed -1p
is not supported on the dev VM, on the assumption that1p
is the sed command we want to run it should besed '1p'
. -
The dev VM is currently pinned to a version of bundler (1.15.1) that shows a warning message if the version of bundler is older than the most recent, and this appears on stdout so it's included in the message. We can disable this with an environment variable
BUNDLE_DISABLE_VERSION_CHECK
.
Putting this all together I had to change this to:
`BUNDLE_DISABLE_VERSION_CHECK=yes bundle show govuk_frontend_toolkit | sed '1p'`
However, this outputs the path twice for some reason:
/usr/lib/rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/govuk_frontend_toolkit-7.0.1
/usr/lib/rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/govuk_frontend_toolkit-7.0.1
If I drop the sed
part entirely then it only outputs the path once, which feels like it's what we're after:
/usr/lib/rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/govuk_frontend_toolkit-7.0.1
rm -rf ${temporary_location}/app/assets | ||
|
||
# Symlink the local | ||
sudo ln -rs ../govuk_frontend_toolkit ${temporary_location}/app/assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my local govuk_frontend_toolkit
checkout I've been running the tests so I have a node_modules
folder which is copied over as part of this command. This folder is huge, and contains coffee script assets. This has two repercussions:
- the requests to static take a very long time as they cache all the files in
node_modules
- the requests to static fail because we don't include the coffee script gem, but there are coffee script files in the node_modules folder which is part of the assets path now and sprockets breaks when trying to load a coffee script processor for them.
If we add a rm -rf ${temporary_location}/app/assets/node_modules
this gets round the problem, but it means we've deleted them from the local checkout to which is pretty surprising. If we change ln -rs
to cp -r
as well that works, but we'd have to keep remembering to run this script whenever we make a local change to govuk_frontend_toolkit.
Not sure what the best solution is TBH.
Closing this since it's a bit hacky and doesnt seem to work everytime |
Due to the way the toolkit is distributed we need to do some tomfoolery to get the local version in static, this makes it easy.