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

chore(deps): update dependency tedious to v8.3.1 #13961

Merged
merged 4 commits into from Jan 19, 2022
Merged

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Jan 16, 2022

WhiteSource Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
tedious 8.3.0 -> 8.3.1 age adoption passing confidence

Release Notes

tediousjs/tedious

v8.3.1

Compare Source

Bug Fixes
  • use proper servername for TLS after redirect (81cb31c)

Configuration

πŸ“… Schedule: At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

β™» Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

πŸ”• Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, click this checkbox.

This PR has been generated by WhiteSource Renovate. View repository job log here.

@WikiRik
Copy link
Member

WikiRik commented Jan 16, 2022

The reason that I opened both of the tedious PRs is that this already gives an error.
Where in 8.3.0 the error messages for reaching a wrong port or IP address include ECONNREFUSED and EHOSTUNREACH respectively, in 8.3.1 both are just 'Failed to connect to x - Could not connect (sequence)' where x is the IP address + port.
Probably this is due to a change in refactoring (see commits here; tediousjs/tedious@v8.3.0...v8.3.1 ).
I will investigate further and once a solution has been found we can look into updating from 8.3.1 to 14.0.0 which seems to be fixing one additional test.

Update: Both in 8.3.0 and in 8.3.1 there are three connections happening of which the last one is the one that we're actually testing. But that happens to the ENOTFOUND test as well, so why that one keeps its specific error message while the others don't is strange.

Update 2: I finally found that the ECONNREFUSED/EHOSTUNREACH/ENOTFOUND error messages are not directly from tedious but from Node itself. I think the issue should then be in tedious' PR 1103 where they changed something regarding the lookup function. But I can't get tedious running per commit so I can't figure it out completely.

@WikiRik WikiRik self-assigned this Jan 16, 2022
@renovate renovate bot force-pushed the renovate/tedious-8.x branch 2 times, most recently from f7ebf16 to 0a0e2f6 Compare January 17, 2022 16:03
@WikiRik
Copy link
Member

WikiRik commented Jan 18, 2022

Okay, so in recap;
The tests for ECONNREFUSED and EHOSTUNREACH fail for the MSSQL Connection Manager
This is because they expect a specific error but tedious returns a generic error that one of the connections in the sequence fails.

So I think the best solution is to refactor that file so all other connections are removed and only the connection tested is 'active' so to say. Now to figure this out how to do it with the 'global' forEach and make sure that we have the wanted connection again after this file has been executed.

@WikiRik
Copy link
Member

WikiRik commented Jan 18, 2022

@ephys I just disabled these tests for now with expiration for the first beta of 7.0.0. I think that updating to the newer version is better since there doesn't seem to be an easy fix to execute this test in isolation. Do you agree?

EDIT: For comparison, the other dialects that tests these error messages in the connection-manager is mariadb. The others don't tests those at all

@ephys
Copy link
Member

ephys commented Jan 19, 2022

All dialects seem to detect the type of connection error to throw a specific error, except db2, and sqlite.

I'm fine with disabling that test. Especially if it's something we have no control over.

Do we want to consider dropping specific ConnectionError subclasses and only throwing a generic "ConnectionError" for all connection issues in v7? With the original error included in the error object. I don't think all these subclasses add much

@WikiRik
Copy link
Member

WikiRik commented Jan 19, 2022

Do we want to consider dropping specific ConnectionError subclasses and only throwing a generic "ConnectionError" for all connection issues in v7? With the original error included in the error object. I don't think all these subclasses add much

I think that some of the subclasses like the AccessDeniedError might be nice but it also depends on the maintainability of these subclasses. We will be promted with this during the beta phase again and then we can discuss further what we want to do

@ephys
Copy link
Member

ephys commented Jan 19, 2022

Even AccessDeniedError is inconsistent. The postgres dialect doesn't make use of it. If users have to write dialect-specific code anyway I'd vote to just throw ConnectionError and ask them to use ConnectionError.original to figure out what went wrong

@WikiRik WikiRik merged commit 2c56aa9 into main Jan 19, 2022
@WikiRik WikiRik deleted the renovate/tedious-8.x branch January 19, 2022 20:06
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

πŸŽ‰ This PR is included in version 7.0.0-alpha.6 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

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.

None yet

3 participants