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

Updating tedious Dependency to 2.1.1 #47

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jschell12
Copy link

@jschell12 jschell12 commented Nov 8, 2017

Description

The tedious dependency has been updated from 1.14.0 to 2.1.1. Unit tests have been modified slightly as they would fail when running against sql server hosted as a container locally.

Related Issue

#43

Motivation and Context

Although this pull request addresses an existing issue to update the tedious pkg, I personally wanted to leverage the new streaming capabilities added since tedious@2.0.1.

How Has This Been Tested?

This change has not affected existing tests. No new tests have been added. I used a docker container running mssql to runt the tests against.

docker run -e 'ACCEPT_EULA=Y' -e 'MSSQL_SA_PASSWORD=Password12!' -p 1433:1433 --name dev -d microsoft/mssql-server-linux:2017-latest

I also ran the following scripts to set up the test environment in sql server

CREATE DATABASE test
CREATE LOGIN test WITH PASSWORD=N'test', DEFAULT_DATABASE=test, CHECK_POLICY=OFF
GRANT ALTER ANY CONNECTION TO test

USE test
CREATE USER test FOR LOGIN test WITH DEFAULT_SCHEMA=dbo
ALTER ROLE db_owner ADD MEMBER test

USE msdb
CREATE USER test FOR LOGIN test WITH DEFAULT_SCHEMA=dbo
ALTER ROLE SQLAgentOperatorRole ADD MEMBER test
ALTER ROLE SQLAgentReaderRole ADD MEMBER test
ALTER ROLE SQLAgentUserRole ADD MEMBER test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Josh Schell added 3 commits November 3, 2017 15:56
…owable. Used sql-server docker container to test against (docker run -e 'ACCEPT_EULA=Y' -e 'MSSQL_SA_PASSWORD=Password12!' -p 1433:1433 --name dev -d microsoft/mssql-server-linux:2017-latest)
@jschell12 jschell12 changed the title Updating [tedious](https://github.com/tediousjs/tedious) to 2.1.1 Updating tedious Dependency to 2.1.1 Nov 8, 2017
@chdh
Copy link

chdh commented Jul 30, 2018

@arthurschreiber @v-suhame
Can you help to get this PR merged? It looks very reasonable.
Without this change, users of the tedious-connection-pool package get the old Tedious version 1.15.0.

@arthurschreiber
Copy link
Contributor

@chdh I'll try and get this merged soon, although I'm not 100% sure about the release process around this package. 👍

@kibertoad
Copy link

@arthurschreiber Any updates?

@arthurschreiber
Copy link
Contributor

@kibertoad as I’m Still on travel until Saturday, I can’t really do much here. Also, upgrading to tedious@2 is a breaking change, as it dropped support for older NodeJS versions and will require a major version bump on this the connection pool library as well.

I’ll try to get to this as soon as I can. 🙇🏻‍♂️

@kibertoad
Copy link

@arthurschreiber Would it be useful if I also created a PR that would drop pre-6 Node support from this library as well?

@mikermcneil
Copy link

@arthurschreiber thanks for following up! I just wanted to check in one more time- if this isn't getting published, no worries, will publish this fork as a scoped package for the time being

mikermcneil referenced this pull request in mikermcneil/tedious-connection-pool Mar 12, 2019
[![Dependency Status](https://david-dm.org/tediousjs/tedious-connection-pool.svg)](https://david-dm.org/tediousjs/tedious-connection-pool)
[![npm version](https://badge.fury.io/js/tedious-connection-pool.svg)](https://badge.fury.io/js/tedious-connection-pool)
[![Build status](https://ci.appveyor.com/api/projects/status/jnurb48ao1wrbgbr?svg=true)](https://ci.appveyor.com/project/ben-page/tedious-connection-pool)
npm version: 1.0.6

Choose a reason for hiding this comment

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

@jschell12 as much as I like getting rid of those stupid badges, I suppose we ought to call out we're doing that in the PR description if we're going to do it

@@ -31,7 +31,7 @@
],
"license": "MIT",
"dependencies": {
"tedious": "^1.14.0"
"tedious": "^2.1.1"

Choose a reason for hiding this comment

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

can we go to 5?

@@ -78,10 +78,10 @@ describe('ConnectionPool', function () {
it('min', function (done) {
this.timeout(timeout);

var poolConfig = {min: 2};
var poolConfig = { min: 2 };

Choose a reason for hiding this comment

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

@jschell12 I think these other changes might be making this harder to merge (it would be for me anyway, if I was the one who had to merge it)

@rlancer
Copy link

rlancer commented Jul 5, 2019

Any reason why this has not been merged?

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

7 participants