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

Changes to mongo atlas #674

Merged
merged 7 commits into from Sep 13, 2018

Conversation

Projects
None yet
8 participants
@Tom5om
Copy link
Contributor

Tom5om commented Aug 6, 2018

Fixes #673

Update database.js to allow users to use a cluster with mongo atlas

Changes to mongo atlas
Update database.js to allow users to use a cluster with mongo atlas
@fega
Copy link

fega left a comment

it works, I already tested it.

@carlozamagni
Copy link

carlozamagni left a comment

I also tested this fix and works fine on my side.

@simison simison added the need: tests label Aug 28, 2018

@simison

This comment has been minimized.

Copy link
Member

simison commented Aug 28, 2018

Thank you! Could you please include a test or two to confirm the change in functionality? :-)

@simison simison requested a review from agenda/maintainers Aug 30, 2018

@wingsbob

This comment has been minimized.

Copy link
Contributor

wingsbob commented Aug 30, 2018

Any chance someone could find a link to connection string spec/documentation? Would make it a lot easier to understand the change, especially as the regex includes a bonus : not mentioned in #673

@simison

This comment has been minimized.

Copy link
Member

simison commented Aug 30, 2018

@shubhanshusingh

This comment has been minimized.

Copy link

shubhanshusingh commented Sep 10, 2018

please solve and merge this

@wingsbob
Copy link
Contributor

wingsbob left a comment

Could do with tests, but doesn't look that trivial to add them for this 😞

4mation and others added some commits Sep 12, 2018

* Moved the regex to its own function
* Added test to check if the regex is valid
Merge pull request #1 from Tom5om/database-atlas
* Moved the regex to its own function

@Tom5om Tom5om dismissed stale reviews from wingsbob, Superjo149, carlozamagni, and fega via 0d700a3 Sep 12, 2018

@Tom5om

This comment has been minimized.

Copy link
Contributor Author

Tom5om commented Sep 12, 2018

Hey @wingsbob and @simison I have updated the code, please review the PR

/**
* Given a mongo connection url the function will check with a regex if it is valid
* @param url
* @returns {AggregationCursor|*|RegExpMatchArray|Promise<Response | undefined>}

This comment has been minimized.

@wingsbob

wingsbob Sep 12, 2018

Contributor

I think this should return boolean?

This comment has been minimized.

@Tom5om

Tom5om Sep 12, 2018

Author Contributor

This was automatically generated when I created the docblock, technically it doesnt return a boolean when it is true

This comment has been minimized.

@wingsbob

wingsbob Sep 12, 2018

Contributor

Since this is now a separate function, I think it might be clearer to call it needsMongoProtocol and always return a boolean?

This comment has been minimized.

@Tom5om

Tom5om Sep 12, 2018

Author Contributor

Aah I like that actually, good idea, as the function checks if it has the mongoProtocol, I have called it hasMongoProtocol

4mation and others added some commits Sep 12, 2018

Merge remote-tracking branch 'origin/database-atlas' into database-atlas
* origin/database-atlas: (47 commits)
  Changes to mongo atlas
  Fix: make `touch` promise-based (#667)
  Update History.md with next version
  Update dependency sinon to v6.1.4 (#663)
  Update dependency sinon to v6 (#642)
  add deps:update to renovate auto labels (#660)
  Update dependency mocha to v5 (#640)
  Add a feature comparison table to readme (#657)
  Feature/update deps (#656)
  Update dependency delay to v3 (#638)
  Mention promises based API in readme intro
  Update known issues in readme
  Update dependency coveralls to v3.0.2 (#643)
  2.0.0
  Update changelog for v2.0.0
  Update yarn lockfile (#654)
  fix(database): use db() syntax rather than pulling dbName from client
  feat(database): upgrade mongodb -> 3.1
  Update dependency sinon to v4.5.0 (#636)
  Update dependency mocha to v4.1.0 (#634)
  ...
@wingsbob
Copy link
Contributor

wingsbob left a comment

lgtm

@wingsbob wingsbob merged commit 759837c into agenda:master Sep 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
security/snyk - package.json (agenda) No new issues
Details
@simison

This comment has been minimized.

Copy link
Member

simison commented Sep 15, 2018

Shipped in Agenda v2.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.