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

fix(datastore): regenerate protos in new namespace #10158

Merged
merged 4 commits into from May 13, 2024

Conversation

codyoss
Copy link
Member

@codyoss codyoss commented May 13, 2024

The namespace and filename need to be different than the file these were copied and regenerated from to make the proto registry not error out on initialization.

Usually it is not good to use a new package name in protos but I believe it is fine here since these are just simple message types being declared and not a service.

Fixes: #10155

The namespace and filename need to be different than the file
these were copied and regenerated from to make the proto registry
not error out on initialization.

Fixes: googleapis#10155
@codyoss codyoss requested review from a team as code owners May 13, 2024 14:28
@codyoss codyoss requested a review from noahdietz May 13, 2024 14:28
@product-auto-label product-auto-label bot added the api: datastore Issues related to the Datastore API. label May 13, 2024
@noahdietz
Copy link
Contributor

While this would fix it, it is also risky. Would it be possible to do a little more brainstorming before pressing the button?

@codyoss
Copy link
Member Author

codyoss commented May 13, 2024

While this would fix it, it is also risky. Would it be possible to do a little more brainstorming before pressing the button?

For sure. I did look at that page before I made the change. I think this should all be safe and non-breaking for from my read. It is not ideal though. In a perfect world those protos would be central defined somewhere and we would import reference them in both places.

It should also be noted that it really valid to use this library in appengine standard according to the docs: https://cloud.google.com/appengine/docs/legacy/standard/go111/datastore#connecting_to_with_app_engine

You cannot use the [Cloud Datastore client library](https://cloud.google.com/datastore/docs/reference/libraries#client-libraries-install-go) with Go applications in the App Engine standard environment.

And in flex I don't think there is value in using the app engine package, that was designed for standard: https://github.com/golang/appengine?tab=readme-ov-file#upgrading-an-app-engine-app-to-the-flexible-environment

@noahdietz
Copy link
Contributor

You cannot use the Cloud Datastore client library with Go applications in the App Engine standard environment.

So is this a feature? 😛

Ok I've reviewed the usage in datastore and this is likely OK...if it weren't the tests should've failed.

@codyoss codyoss added the automerge Merge the pull request once unit tests and other checks pass. label May 13, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit 8875511 into googleapis:main May 13, 2024
8 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datastore: protobuf namespace conflict with google.golang.org/appengine/v2
2 participants