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

Fixed test SSLVerifyHostName when build NATS_FORCE_HOST_VERIFICATION=OFF #788

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Aug 9, 2024

This test would fail unless we force host verification, so adapt test to take into consideration the expected result based on the build environment variable.

Signed-off-by: Ivan Kozlovic [email protected]

This test would fail unless we force host verification, so adapt
test to take into consideration the expected result based on the
build environment variable.

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic kozlovic requested a review from levb August 9, 2024 16:24
@kozlovic
Copy link
Member Author

kozlovic commented Aug 9, 2024

@levb We already have quite a bit of a matrix (even within GA I mean), but do you think we need to have a new one to test with the force_host_verification disabled? It is enabled by default and I would want the lib to be like that by default, but yeah, building with it disabled showed that I did not account for that originally in that test.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.65%. Comparing base (1553d4a) to head (b87fdca).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
+ Coverage   68.71%   70.65%   +1.94%     
==========================================
  Files          39       47       +8     
  Lines       15207    15136      -71     
  Branches     3143     3090      -53     
==========================================
+ Hits        10449    10694     +245     
+ Misses       1700     1458     -242     
+ Partials     3058     2984      -74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@levb levb left a comment

Choose a reason for hiding this comment

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

LGTM;

I added a GHA job and pushed here, hope you don't mind.

@levb
Copy link
Collaborator

levb commented Aug 9, 2024

@kozlovic I am now looking at the https://github.com/nats-io/nats.c/actions/runs/10322972901/job/28579373546?pr=788 failure, will send a separate PR for it, seems another test (args race) issue

@levb levb merged commit 4f19da6 into main Aug 13, 2024
30 checks passed
@levb levb deleted the fix_test_ssl_verify_hostname branch August 13, 2024 16:34
github-actions bot pushed a commit that referenced this pull request Aug 13, 2024
github-actions bot pushed a commit to levb/nats.c that referenced this pull request Aug 14, 2024
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.

2 participants