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

feat(bigquerycontrol): generate library #14440

Conversation

scotthart
Copy link
Member

@scotthart scotthart commented Jul 8, 2024

This change is Reviewable

@scotthart scotthart requested a review from a team as a code owner July 8, 2024 21:52
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.59%. Comparing base (d43e267) to head (c069781).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14440   +/-   ##
=======================================
  Coverage   93.59%   93.59%           
=======================================
  Files        2315     2315           
  Lines      207150   207150           
=======================================
+ Hits       193876   193880    +4     
+ Misses      13274    13270    -4     

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

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

I don't agree with the extra layer of nesting in the product_paths, but maybe I am missing something?

Other than that LGTM... Exciting!

# BigQueryControl
service {
service_proto_path: "google/cloud/bigquery/v2/dataset.proto"
product_path: "google/cloud/bigquerycontrol/dataset/v2"
Copy link
Member

Choose a reason for hiding this comment

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

For all of these, why not google/cloud/bigquerycontrol/v2 ?

It seems simpler and less verbose to me.

CHANGELOG.md Outdated
Comment on lines 39 to 40
- [Cloud BigQuery Control API](/google/cloud/bigquerycontrol/README.md) -
This library provides support for the Dataset, Job, Model, Project, Routine, RowAccessPolicy, and Table REST resources.
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you align this to 80 characters?

We turned off mdformat-ing the changelog because I didn't like how some things were rendered in GH's release notes.

CHANGELOG.md Outdated
@@ -32,6 +32,13 @@ added to your `.bazelrc` file.
build --@io_opentelemetry_cpp//api:with_abseil
```

### New Libraries
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this section above OpenTelemetry ?


\[cloud-service-docs\]: https://cloud.google.com/bigquerycontrol \[EDIT HERE\]
Copy link
Member

Choose a reason for hiding this comment

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

comment: Weird. I guess when a service doesn't have that product documentation field in the yaml, we get an [EDIT HERE] marker which confuses checkers.

set(DOXYGEN_PROJECT_NAME "Cloud BigQuery Control API C++ Client")
set(DOXYGEN_PROJECT_BRIEF
"A C++ Client Library for the Cloud BigQuery Control API")
set(DOXYGEN_PROJECT_NUMBER "${PROJECT_VERSION}")
Copy link
Member

Choose a reason for hiding this comment

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

comment: we used to add (Experimental)...

if (_opt_EXPERIMENTAL)
set(DOXYGEN_PROJECT_NUMBER "${PROJECT_VERSION} (Experimental)")

... but honestly, I don't think this field makes it to cloud.google.com/cpp. I think it is a relic.

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 215 files reviewed, 3 unresolved discussions (waiting on @dbolduc)

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

For all of these, why not google/cloud/bigquerycontrol/v2 ?

It seems simpler and less verbose to me.

The google/cloud/<product>/<service>/<version> format is the convention we've been using in the rest of the repo, so I followed that.: e.g. google/cloud/bigquery/storage/v1. It is possible that versions of each service could change independently.



CHANGELOG.md line 35 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

nit: move this section above OpenTelemetry ?

Done


CHANGELOG.md line 40 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

nit: can you align this to 80 characters?

We turned off mdformat-ing the changelog because I didn't like how some things were rendered in GH's release notes.

Done


google/cloud/bigquerycontrol/CMakeLists.txt line 23 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

comment: we used to add (Experimental)...

if (_opt_EXPERIMENTAL)
set(DOXYGEN_PROJECT_NUMBER "${PROJECT_VERSION} (Experimental)")

... but honestly, I don't think this field makes it to cloud.google.com/cpp. I think it is a relic.

Added in case we decide we want it.


google/cloud/bigquerycontrol/README.md line 0 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

comment: Weird. I guess when a service doesn't have that product documentation field in the yaml, we get an [EDIT HERE] marker which confuses checkers.

Done.

Copy link
Member

@dbolduc dbolduc 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 215 files reviewed, 3 unresolved discussions (waiting on @scotthart)

a discussion (no related file):

google/cloud/<product>/<service>/<version> format is the convention we've been using in the rest of the repo

I don't think so. That is the convention the BigQuery service was using, but they changed it with these v2 protos.

I would think of it as mapping:

google/cloud/bigquery/ in googleapis to google/cloud/bigquery/ in google-cloud-cpp.

https://github.com/googleapis/googleapis/tree/master/google/cloud/bigquery

If these were gRPC services, we would have put them in our google/cloud/bigquery/v2.

But we can't. So we should map google/cloud/bigquery in googleapis to google/cloud/bigquerycontrol in google-cloud-cpp for the REST based services.


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 215 files reviewed, 3 unresolved discussions (waiting on @dbolduc)

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

google/cloud/<product>/<service>/<version> format is the convention we've been using in the rest of the repo

I don't think so. That is the convention the BigQuery service was using, but they changed it with these v2 protos.

I would think of it as mapping:

google/cloud/bigquery/ in googleapis to google/cloud/bigquery/ in google-cloud-cpp.

https://github.com/googleapis/googleapis/tree/master/google/cloud/bigquery

If these were gRPC services, we would have put them in our google/cloud/bigquery/v2.

But we can't. So we should map google/cloud/bigquery in googleapis to google/cloud/bigquerycontrol in google-cloud-cpp for the REST based services.

Ok, I'm convinced. I'll likely create a new PR as this will be a large structural change.


@scotthart
Copy link
Member Author

superceded by #14449

@scotthart scotthart closed this Jul 9, 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