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

chore(internal): stop generating Ads packages in genproto #3214

Merged
merged 2 commits into from
Nov 16, 2020

Conversation

tbpg
Copy link
Contributor

@tbpg tbpg commented Nov 16, 2020

These packages don't have a corresponding client library (in
cloud.google.com/go or another module). They aren't ready for production
use. So, we're going to stop generating them for now and will revisit
in the future when ready.

A go-genproto PR will remove the existing Ads packages.

If needed, these packages can be generated locally or as part of a build
process. However, we're not ready to generate and publish them
automatically.

@AnashOommen @aohren

These packages don't have a corresponding client library (in
cloud.google.com/go or another module). They aren't ready for production
use. So, we're going to stop generating them for now and will revisit
in the future when ready.

A go-genproto PR will remove the existing Ads packages.

If needed, these packages can be generated locally or as part of a build
process. However, we're not ready to generate and publish them
automatically.
@tbpg tbpg requested a review from a team as a code owner November 16, 2020 20:13
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2020
@aohren
Copy link

aohren commented Nov 16, 2020

LGTM

tbpg added a commit to googleapis/go-genproto that referenced this pull request Nov 16, 2020
These packages do not have a corresponding client library that uses them
and are untested with the production API. Therefore, they are not ready
for production use.

In the future, we will investigate the best way to generate these APIs
while making sure they have a high level of reliability.

If needed, you can use protoc to generate Go packages for these protos
locally or during your build process. You can also pin to an older
version of this module; however, you won't be able to use updates to
other APIs in this module.

See googleapis/google-cloud-go#3214 for the
corresponding update to stop updating/generating these packages.
@@ -110,7 +123,7 @@ func regenGenproto(ctx context.Context, genprotoDir, googleapisDir, protoDir str
// Run protoc on all protos of all packages.
grp, _ := errgroup.WithContext(ctx)
for pkg, fnames := range pkgFiles {
if !strings.HasPrefix(pkg, "google.golang.org/genproto") || denylist[pkg] {
if !strings.HasPrefix(pkg, "google.golang.org/genproto") || denylist[pkg] || hasPrefix(pkg, skipPrefixes) {
Copy link
Member

Choose a reason for hiding this comment

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

If skipPrefixes gets larger, it would be more efficient to preemptively remove strings that match the prefix from pkgFiles rather than checking it everytime in this for loop. Considering the size of skipPrefixes for now, I don't think it's a blocking optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on both counts. Going for simplicity for now.

Copy link
Contributor Author

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -110,7 +123,7 @@ func regenGenproto(ctx context.Context, genprotoDir, googleapisDir, protoDir str
// Run protoc on all protos of all packages.
grp, _ := errgroup.WithContext(ctx)
for pkg, fnames := range pkgFiles {
if !strings.HasPrefix(pkg, "google.golang.org/genproto") || denylist[pkg] {
if !strings.HasPrefix(pkg, "google.golang.org/genproto") || denylist[pkg] || hasPrefix(pkg, skipPrefixes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on both counts. Going for simplicity for now.

@tbpg tbpg added the automerge Merge the pull request once unit tests and other checks pass. label Nov 16, 2020
tbpg added a commit to googleapis/go-genproto that referenced this pull request Nov 16, 2020
These packages do not have a corresponding client library that uses them
and are untested with the production API. Therefore, they are not ready
for production use.

In the future, we will investigate the best way to generate these APIs
while making sure they have a high level of reliability.

If needed, you can use protoc to generate Go packages for these protos
locally or during your build process. You can also pin to an older
version of this module; however, you won't be able to use updates to
other APIs in this module.

See googleapis/google-cloud-go#3214 for the
corresponding update to stop updating/generating these packages.
@gcf-merge-on-green gcf-merge-on-green bot merged commit 432f2c2 into master Nov 16, 2020
@gcf-merge-on-green gcf-merge-on-green bot deleted the ads branch November 16, 2020 21:02
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants