Skip to content

Conversation

@scotthart
Copy link
Member

@scotthart scotthart commented Apr 3, 2025

This change is Reviewable

@codecov
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 92.95775% with 10 lines in your changes missing coverage. Please review.

Project coverage is 92.90%. Comparing base (38e18a8) to head (f8785d3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/ssl_certificate.h 64.70% 6 Missing ⚠️
...e/tests/mtls_object_basic_crud_integration_test.cc 96.07% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #15062    +/-   ##
========================================
  Coverage   92.90%   92.90%            
========================================
  Files        2354     2356     +2     
  Lines      210365   210507   +142     
========================================
+ Hits       195442   195576   +134     
- Misses      14923    14931     +8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

/**
* Represents an SSL certificate used in TLS authentication.
*/
class SslCertificate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea to encapsulate the properties in a class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did so hoping that we can reuse this for future features.

if (!status.ok()) return OnTransferError(context, std::move(status));
}

#if CURL_AT_LEAST_VERSION(7, 71, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this issue some kind of warning if the version is below the threshold?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an error if they try and use this feature with an older version of libcurl

Copy link
Member Author

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @ddelgrosso1)

/**
* Represents an SSL certificate used in TLS authentication.
*/
class SslCertificate {
Copy link
Member Author

Choose a reason for hiding this comment

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

I did so hoping that we can reuse this for future features.

if (!status.ok()) return OnTransferError(context, std::move(status));
}

#if CURL_AT_LEAST_VERSION(7, 71, 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

I added an error if they try and use this feature with an older version of libcurl

@scotthart scotthart merged commit 12d9697 into googleapis:main Apr 10, 2025
74 of 77 checks passed
devbww pushed a commit to devbww/google-cloud-cpp that referenced this pull request Apr 25, 2025
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