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

feat!: migrate to v2.0.0 #147

merged 99 commits into from Nov 11, 2020

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 label Oct 2, 2020
@larkee larkee requested a review from busunkim96 Oct 6, 2020
@busunkim96 busunkim96 requested a review from software-dov Oct 7, 2020
@busunkim96
Copy link
Collaborator

@busunkim96 busunkim96 commented Oct 7, 2020

Thank you for working on this!

@larkee
Copy link
Contributor Author

@larkee 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
Collaborator

@busunkim96 busunkim96 left a comment

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
@larkee larkee requested a review from busunkim96 Oct 26, 2020
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 Nov 5, 2020
@larkee larkee requested review from as code owners Nov 5, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

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
10 checks passed
@release-please release-please bot mentioned this pull request Nov 11, 2020
@wyardley
Copy link

@wyardley 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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants