Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Progress on supporting IPv6#805

Merged
jsumners merged 3 commits intoldapjs:masterfrom
MaxLeiter:node18
Jun 8, 2022
Merged

Progress on supporting IPv6#805
jsumners merged 3 commits intoldapjs:masterfrom
MaxLeiter:node18

Conversation

@MaxLeiter
Copy link
Copy Markdown

@MaxLeiter MaxLeiter commented Jun 1, 2022

Resolves #804

@MaxLeiter MaxLeiter marked this pull request as draft June 1, 2022 22:48
@MaxLeiter MaxLeiter mentioned this pull request Jun 1, 2022
Comment on lines +515 to +517
if (addr.family === 'IPv6' || addr.family === 6) {
host = '[' + this.host + ']'
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need a test to check for IPv6 connectivity. Since we don't yet have v18 in the testing matrix, we don't need to cover the branch. I don't want to add v18 to the testing matrix until I can cut the next major release.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@MaxLeiter are you able to add this test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah sorry, I misunderstood what you meant by “dont need to cover this test”

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Test pushed. Lmk if that's what you meant.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Suites:   ​54 passed​, ​54 of 54 completed​
Asserts:  ​​​2782 passed​, ​of 2782​

Co-authored-by: James Sumners <james@sumners.email>
@MaxLeiter MaxLeiter marked this pull request as ready for review June 3, 2022 01:45
@MaxLeiter
Copy link
Copy Markdown
Author

Marking this as ready as tests pass locally

@jsumners
Copy link
Copy Markdown
Member

jsumners commented Jun 3, 2022

Marking this as ready as tests pass locally

It is missing the test requested in #805 (comment)

@MaxLeiter MaxLeiter requested a review from jsumners June 7, 2022 20:09
Copy link
Copy Markdown
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jsumners jsumners merged commit 188870d into ldapjs:master Jun 8, 2022
@MaxLeiter MaxLeiter deleted the node18 branch June 8, 2022 01:00
@jsumners
Copy link
Copy Markdown
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node 18 support

3 participants