tedious-connection-pool icon indicating copy to clipboard operation
tedious-connection-pool copied to clipboard

Updating tedious Dependency to 2.1.1

Open jschell12 opened this issue 7 years ago • 7 comments

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 [email protected].

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)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [x] 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.
  • [x] All new and existing tests passed.

jschell12 avatar Nov 08 '17 17:11 jschell12

@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.

chdh avatar Jul 30 '18 01:07 chdh

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

arthurschreiber avatar Aug 04 '18 08:08 arthurschreiber

@arthurschreiber Any updates?

kibertoad avatar Sep 22 '18 14:09 kibertoad

@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. 🙇🏻‍♂️

arthurschreiber avatar Sep 27 '18 03:09 arthurschreiber

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

kibertoad avatar Sep 27 '18 07:09 kibertoad

@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 avatar Mar 11 '19 23:03 mikermcneil

Any reason why this has not been merged?

rlancer avatar Jul 05 '19 21:07 rlancer