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

Fix failing Heroku review app #833

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Fix failing Heroku review app #833

merged 2 commits into from
Jun 26, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Jun 25, 2018

In #827 we removed pinning the node version, instead specifying the minimum version.

This resulted in Heroku failing as there is an error in the gulp taks in node 10.5.0 Heroku installs.

Until we decide to upgrade to Gulp 4 we need to pin the version in package.json.

Alternative option is to remove version entirely, although we want to set that for anyone wanting to contribute using yarn instead of npm.

Heroku used Node 10.5, built and deployed correctly

Installing binaries
engines.node (package.json):  >= 8.9.4
engines.npm (package.json):   unspecified (use default)
       
Resolving node version >= 8.9.4...
Downloading and installing node 10.5.0...
Using default npm version: 6.1.0

Caching build
       Clearing previous node cache
       Skipping cache save (disabled by config)
-----> Pruning devDependencies
       Skipping because NPM_CONFIG_PRODUCTION is 'false'
-----> Build succeeded!
-----> Discovering process types
       Procfile declares types -> web
-----> Compressing...
       Done: 144.1M
-----> Launching...
       Released v517
       https://govuk-frontend-review.herokuapp.com/ deployed to Heroku

When requesting the site it threw a 503 error

2018-06-25T15:15:57.649864+00:00 heroku[router]: at=error code=H10 desc="App crashed" method=GET path="/favicon.ico" host=govuk-frontend-review.herokuapp.com request_id=180da83d-7c96-4c99-9a4e-52830e670538 fwd="195.89.171.5" dyno= connect= service= status=503 bytes= protocol=http

Firstly we tried removing the space between number and greaterOrEqual sign, but that didn't work.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-833 June 25, 2018 15:14 Inactive
@NickColley
Copy link
Contributor

NickColley commented Jun 25, 2018

Can you add context to this commit that explains why we are pinning it again please?

@kr8n3r kr8n3r force-pushed the fix-failing-heroku-deployment branch from fbb3ffe to a7fd701 Compare June 25, 2018 15:20
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-833 June 25, 2018 15:20 Inactive
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Needs commit that explains why we need to pin

And changelog entry until we make a decision on #834

@kr8n3r
Copy link
Author

kr8n3r commented Jun 26, 2018

Update 10.5.0 fails with this

gulp[88868]: ../src/node_contextify.cc:629:static void node::contextify::ContextifyScript::New(const FunctionCallbackInfo<v8::Value> &): Assertion `args[1]->IsString()' failed.
 1: 0x100033c31 node::Abort() [/usr/local/bin/node]
 2: 0x100032c77 node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, char const*, int, v8::Local<v8::Value>*, node::async_context) [/usr/local/bin/node]
 3: 0x100058a4b node::contextify::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
 4: 0x10022822f v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) [/usr/local/bin/node]
 5: 0x1002273c8 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/bin/node]
 6: 0x100226d97 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node]
 7: 0x3eb7788041bd
sh: line 1: 88868 Abort trap: 6           gulp copy-assets

and i found this cloudfour/core-gulp-tasks#63

Update to Gulp 4 in firebreak?

In #827 we removed pinning teh node version,
instead specifiying the minimum version.

This resulted in Heroku failing as there is an error
in the gulp taks in node 10.5.0 Heroku installs.

Until we decide to upgrade to Gulp 4 we need to
pin the version in package.json.

Alternative option is to remove version entirely, although
we want to set that for anyone wanting to contribute using
`yarn` instead of `npm`.
@kr8n3r kr8n3r force-pushed the fix-failing-heroku-deployment branch from a7fd701 to 20afba3 Compare June 26, 2018 08:13
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-833 June 26, 2018 08:13 Inactive
@kr8n3r kr8n3r changed the title Fix failing Heroku deployment Fix failing Heroku app Jun 26, 2018
@kr8n3r kr8n3r changed the title Fix failing Heroku app Fix failing Heroku review app Jun 26, 2018
@kr8n3r
Copy link
Author

kr8n3r commented Jun 26, 2018

updated with requested changes

@NickColley
Copy link
Contributor

Great work, thanks Jani

@kr8n3r kr8n3r merged commit 9069348 into master Jun 26, 2018
@kr8n3r kr8n3r deleted the fix-failing-heroku-deployment branch June 26, 2018 11:05
@NickColley NickColley mentioned this pull request Jul 13, 2018
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.

None yet

3 participants