Skip to content
This repository has been archived by the owner on Jun 28, 2021. It is now read-only.

Tooling cleanup #466

Merged
merged 14 commits into from
Aug 29, 2016
Merged

Tooling cleanup #466

merged 14 commits into from
Aug 29, 2016

Conversation

ahmedelgabri
Copy link
Contributor

@thabti thabti added the WIP label Aug 27, 2016
@mmahalwy
Copy link
Contributor

Problem coming from phantomjs

Would you mind updating our phantomjs package?

@ahmedelgabri
Copy link
Contributor Author

ahmedelgabri commented Aug 27, 2016

@mmahalwy That's not phamtomjs problem it's a node-sass issue. Updated it.

Also, updated how travis pickup the right node version since it was using node 5

add `.nvrmc` for two reasons:

- travis uses `nvm` by default to manage node versions.
- It will make it easy for contributers who use `nvm` to use the proper
node version locally too.
@ahmedre
Copy link
Contributor

ahmedre commented Aug 27, 2016

jenkins does use 6.3 (it uses the same configuration as the Dockerfile).

complaint from Jenkins is:

> [email protected] build:client /quran
> node webpack --config webpack/prod.config.js

module.js:442
    throw err;
    ^

Error: Cannot find module '/quran/webpack'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.runMain (module.js:575:10)
    at run (bootstrap_node.js:352:7)
    at startup (bootstrap_node.js:144:9)
    at bootstrap_node.js:467:3

jenkins build using prod.

@ahmedelgabri
Copy link
Contributor Author

Thanks for posting this because I can't see the errors myself, will have a look at this tomorrow.

@@ -1,8 +1,6 @@
sudo: false
language: node_js
env: TEST_SUITE=unit
node_js:
- "5.6.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

i prefer putting 6.3.0 here instead of .nvmrc just because it will save a lot of pain in the future (next time Travis breaks, people will immediately open .travis.yml, and it may not be obvious to someone else that they need to look at .nvmrc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add a comment too saying that the node version is handled in .nvmrc.

Also most of the time travis breaks it will be something else plus travis already shows the version in the log & also that it picks it up from nvm.

I'm planning to make the tools & build process more robust so that things doesn't break unless it's something major.

@ahmedre
Copy link
Contributor

ahmedre commented Aug 28, 2016

Deployed to: http://staging.quran.com:32839

@ahmedre
Copy link
Contributor

ahmedre commented Aug 28, 2016

Deployed to: http://staging.quran.com:32840

@ahmedre
Copy link
Contributor

ahmedre commented Aug 28, 2016

Deployed to: http://staging.quran.com:32841

@thabti
Copy link
Contributor

thabti commented Aug 28, 2016

Looks good to me. let me know if this is ready to be merged.

@@ -18,7 +18,7 @@ RUN cp /etc/cron.daily/logrotate /etc/cron.hourly
# cache npm install when package.json hasn't changed
WORKDIR /tmp
ADD package.json package.json
RUN npm install
RUN npm install --silent --no-progress
Copy link
Contributor

@mmahalwy mmahalwy Aug 29, 2016

Choose a reason for hiding this comment

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

technically, we should eventually add --production to it. So we aren't downloading unneeded dependencies like phantomjs or karma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but then travis will fail because the karma test that runs on travis requires phantomjs

@ahmedre
Copy link
Contributor

ahmedre commented Aug 29, 2016

Deployed to: http://staging.quran.com:32845

@ahmedelgabri
Copy link
Contributor Author

@sabeurthabti It's ok form my side too, but to be sure I will just do one last check today afternoon.

@ahmedre
Copy link
Contributor

ahmedre commented Aug 29, 2016

Deployed to: http://staging.quran.com:32846

@ahmedelgabri
Copy link
Contributor Author

@sabeurthabti You can merge this now. I'll follow up with a new PR to do more cleanup, but for now this is good to go 👍

@thabti
Copy link
Contributor

thabti commented Aug 29, 2016

lgtm

@thabti thabti merged commit 7f57ba1 into quran:master Aug 29, 2016
@thabti
Copy link
Contributor

thabti commented Aug 29, 2016

Thank you @ahmedelgabri

@mmahalwy
Copy link
Contributor

Thanks @ahmedelgabri !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants