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!: migrate to v2.0.0 #147

Merged
merged 99 commits into from Nov 11, 2020
Merged

Conversation

larkee
Copy link
Contributor

@larkee larkee commented Oct 2, 2020

This PR is a preview of the 2.0 release.

TODO list:

  • Fix docs.

  • Describe changes in UPGRADING guide.

BREAKING CHANGES

  • list_instances, list_databases, list_instance_configs, and list_backups will now return protos rather than the handwritten wrapper

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2020
@larkee larkee requested a review from busunkim96 October 6, 2020 02:31
@busunkim96
Copy link
Contributor

Thank you for working on this!

@larkee
Copy link
Contributor Author

larkee commented Oct 7, 2020

  • I noticed there doesn't seem to be a samples presubmit. I opened 335755839 to add those kokoro configs internally.

Thanks! I wasn't sure what was missing.

Handwritten files have been moved from google/cloud/spanner_v1 to google/cloud/spanner

I believe the other handwritten/generated mixes keep the generated surface in the versioned directory. What is the
motivation for the change?
https://github.com/googleapis/python-firestore/tree/master/google/cloud/firestore_v1
https://github.com/googleapis/python-pubsub/tree/v2.1.0/google/cloud/pubsub_v1

The motivation was two fold. It separates the handwritten surfaces which makes it clearer to contributes what is written and what is generated. It also prevents having to customize the generated google/cloud/spanner_v1/__init__.py. However given that this file shouldn't change too much, it's probably reasonable to exclude it from generation and manually update it when required.

I'm happy to revert these files back to their original folder if you think that would be best.

  • I'm not familiar with the Spanner client, are there particular files we should direct our attention to?

Most of the handwritten surfaces are used directly by users. The major ones are:

  • client.py
  • instance.py
  • database.py
  • backup.py
  • `batch.py
  • snapshot.py
  • transaction.py

However, the files that changed the most during migration are:

  • _helpers.py
  • snapshot.py
  • streamed.py
  • transaction.py

Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

I would prefer the handwritten code live in the versioned directory for consistency with the other libraries. It looks like firestore excludes __init__.py in synth.py https://github.com/googleapis/python-firestore/blob/c3acd4a04745c93edb2f61bf9be6fa33f439f4b0/synth.py#L41

Fully generated libraries use the unversioned path google/cloud/{api}/ for the default alias (equivalent of google/cloud/spanner.py) https://github.com/googleapis/python-speech/blob/master/google/cloud/speech/__init__.py so there's also a small chance for confusion there.

google/cloud/spanner/snapshot.py Outdated Show resolved Hide resolved
google/cloud/spanner.py Outdated Show resolved Hide resolved
google/cloud/spanner.py Show resolved Hide resolved
google/cloud/spanner_v1/streamed.py Outdated Show resolved Hide resolved
@larkee larkee marked this pull request as ready for review November 5, 2020 07:16
@larkee larkee requested review from a team as code owners November 5, 2020 07:16
Copy link
Contributor

@skuruppu skuruppu left a comment

Choose a reason for hiding this comment

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

I only reviewed UPGRADING.md so my LGTM is for that file.

UPGRADING.md Outdated Show resolved Hide resolved
@c24t c24t mentioned this pull request Nov 10, 2020
@larkee larkee merged commit bf4b278 into googleapis:master Nov 11, 2020
@release-please release-please bot mentioned this pull request Nov 11, 2020
@wyardley
Copy link

wyardley commented Dec 1, 2020

@larkee: In the end, the "breaking change" that shows up in the changelog was
migrate to v2.0.0 (#147) vs

list_instances, list_databases, list_instance_configs, and list_backups will now return protos rather than the handwritten wrapper

which would have been much more useful

@larkee
Copy link
Contributor Author

larkee commented Dec 2, 2020

@wyardley My apologies! I'll do my best to label changes better in future 👍 I'll update the changelog directly to show the more useful message

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.

None yet

4 participants