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

Improving mongoose->mongodb dependency chain requirement. #70

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Improving mongoose->mongodb dependency chain requirement. #70

wants to merge 1 commit into from

Conversation

vinnylinck
Copy link

After starting a project from scratch, I started to receive the following error message:

Wrong adapter name, see docs for built in adapter

Actually that happens because SSE adapter assumes that required mongodb module is located inside mongoose folder tree. This is a bad practice because if the target project has it as a dependency, it won't be downloaded into mongoose node-modules folder.

So, in order to make this "friendly" for dev community and save some hours of debugging for who is beginning with HH, I'm submitting two improvements:
#1: Add mongodb as module as a dependency for hapi-harvester and require it directly, w/o a specific path (to avoid issues when it was required already).
#2: Changed the error message when loading adapters to be friendly (because it induces the wrong direction when debugging). The matter fact is that exception is thrown when HH can't load an adapter, because it doesn't exist or because an error occurred. Still wondering how to expose the error message to tell the developer what happened exactly.

@kristofsajdak
Copy link
Contributor

Hey @vinnylinck, I appreciate the PR however the Travis build is failing.

I did some investigation and compared it with a local test run, the root cause seems to be that the newest mongo 3.2.4 image ( we reference mongo:3 ) depends on a parent image which doesn't have curl installed.

adding the snippet below in both setup.sh and query.sh should fix the issue

apt-get update
apt-get install -y curl

There might be a more elegant solution to this issue, feel free to propose if you see an alternative

@kristofsajdak kristofsajdak self-assigned this Mar 13, 2016
@vinnylinck
Copy link
Author

Thanks. I'll spend some time on CI failures as soon as I have some time.

@mavdi
Copy link
Contributor

mavdi commented Apr 3, 2017

@vinnylinck Are you able to revive this PR?

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

Successfully merging this pull request may close these issues.

3 participants