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.

Sign up for GitHub

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

Jump to bottom

tls_available option #694

Merged
merged 5 commits into from
Nov 13, 2023
Merged

tls_available option #694

merged 5 commits into from
Nov 13, 2023

Conversation

biot2
Copy link
Contributor

@biot2 biot2 commented Nov 11, 2023 •
edited
Loading

  1. Added tls_available parameter understanding of INFO structure.
  2. Fixed wrongful security option check causing incorrect error code NATS_SECURE_CONNECTION_WANTED

@biot2
Update natsp.h
341dc81 
Added  __natsServerInfo.tlsAvailable
@biot2
Update conn.c
7607b24 
- Added understanding of tls_available INFO param
- Fixed wrongfully checking secure option and returning wrong error 
code NATS_SECURE_CONNECTION_WANTED
@biot2
Merge pull request #1 from biot2/biot2-patch-1
ea2cb00 
Update natsp.h
@biot2
Merge pull request #2 from biot2/biot2-patch-2
cf7a01e 
Update conn.c
@biot2
Merge branch 'nats-io:main' into main
d1813db
@biot2 biot2 closed this Nov 13, 2023
@biot2 biot2 reopened this Nov 13, 2023
@biot2
Copy link
Contributor Author

biot2 commented Nov 13, 2023

@kozlovic

kozlovic
kozlovic approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. It would have been nice to add the test that showed the failure to connect prior to the fix and verify that it passes now. But we can add that later. Thank you for your contribution.

@kozlovic kozlovic merged commit c482b0b into nats-io:main Nov 13, 2023
1 check passed
@biot2
Copy link
Contributor Author

biot2 commented Nov 14, 2023

LGTM. It would have been nice to add the test that showed the failure to connect prior to the fix and verify that it passes now. But we can add that later. Thank you for your contribution.

You're welcome. Actually the test module is so big and a bit complicated. I will try to add the test case and make sure everything else is as it should be.

@kozlovic
Copy link
Member

@biot2 I meant that maintainers could add the test, you don't have to especially if you are new to the project. I have added the test myself, you can see PR #697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlovic kozlovic kozlovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@biot2 @kozlovic