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

Upgrade dependencies to gapic-* 0.9.x packages #2324

Merged
merged 4 commits into from
Sep 16, 2016

Conversation

bjwatson
Copy link

@bjwatson bjwatson commented Sep 16, 2016

@dhermes These are the new dependencies for #2258 (and that you need for the 0.19.0 release of google-cloud-python).

Please note that we changed the names of the packages from gax-* to gapic-*. It was a mistake to call them gax-* (that just refers to the google-gax library), and should have always called them gapic-* instead.

Fixes #2258.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 16, 2016
@tseaver tseaver added packaging do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Sep 16, 2016
@tseaver tseaver changed the title DO NOT MERGE: Upgrade dependencies to gapic-* 0.9.x packages Upgrade dependencies to gapic-* 0.9.x packages Sep 16, 2016
@bjwatson
Copy link
Author

Thanks for updating this ticket with labels @tseaver.

I just realized that I forgot to update the import statements. Just pushed another commit to do that.

@tseaver
Copy link
Contributor

tseaver commented Sep 16, 2016

@bjwatson this change is hard for us to evaluate / test without updating the machinery to install those deps from https://testpypi.python.org/pypi. Maybe add a temporary command to the tox.ini which pre-installs them from there?

@daspecster
Copy link
Contributor

@bjwatson, sorry I didn't read your original comment closely.

Adding...

commands =
    pip install -i https://testpypi.python.org/pypi <package name>

to the tox environments, as @tseaver mentioned, should get this running.

@bjwatson
Copy link
Author

I realized that by setting upper bounds on the 0.8.x version of the gax-google-* packages, that I could safely push the new 0.9.x packages to PyPI. I've just done that, and am now running these tests.

@bjwatson bjwatson removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 16, 2016
@bjwatson
Copy link
Author

This is ready to merge once the tests pass. I'm not familiar enough with them to know why they're breaking. Could someone take a look and let me know if/what I broke?

I'll be AFK for an hour, but will check back in after that. Thanks!

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

@bjwatson For future reference, tox has support for multiple index servers.

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

I'm looking at the breakages now

@daspecster
Copy link
Contributor

daspecster commented Sep 16, 2016

@dhermes https://travis-ci.org/GoogleCloudPlatform/google-cloud-python/builds/160481390#L851-L856

.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/logging/v2/config_service_v2_api.py             60     29      4      0    48%   75, 80, 96, 109, 122, 157-191, 236-238, 262-263, 293-295, 328-330, 351-352
.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/logging/v2/logging_service_v2_api.py            62     30      8      0    46%   77, 82, 98, 111, 124, 159-191, 216-217, 265-275, 335-340, 380-382
.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/logging/v2/metrics_service_v2_api.py            60     29      4      0    48%   73, 78, 94, 107, 120, 155-189, 234-236, 260-262, 292-294, 327-329, 350-352
.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/pubsub/v1/__init__.py                            0      0      0      0   100%
.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/pubsub/v1/publisher_api.py                      66     32      4      0    49%   85, 90, 106, 119, 132, 167-205, 235-236, 266-267, 290-291, 333-335, 377-379, 403-404
.tox/cover/lib/python2.7/site-packages/google/cloud/gapic/pubsub/v1/subscriber_api.py                     81     43      6      0    44%   75, 80, 88, 104, 117, 131, 145, 158, 193-237, 302-309, 332-333, 375-377, 401-403, 438-442, 470-472, 510-514, 546-548

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 16, 2016
@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

I just pushed a commit to @bjwatson's branch to fix the coverage issue. Really cool new GitHub feature allowing maintainers to push commits to PRs!

@jonparrott Where to file bugs for the CLA-bot?

'gapic-google-pubsub-v1 >= 0.9.0, < 0.10dev',
'grpc-google-pubsub-v1 >= 0.9.0, < 0.10dev',
'gapic-google-logging-v2 >= 0.9.0, < 0.10dev',
'grpc-google-logging-v2 >= 0.9.0, < 0.10dev',

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

@bjwatson This LGTM though a rebase may be important here to make sure we actually still cover all imports. Rather than a rebase you could just re-start this branch from HEAD locally and then git merge --squash origin/breaking-gapic-changes onto it. This also has the benefit of removing my commit and making CLA-bot happy.

@tseaver
Copy link
Contributor

tseaver commented Sep 16, 2016

@dhermes the indexserver directive docs state that the directive is deprecated. :(

Also, we could have worked around the issue better in tox.ini before #2308: relying on pip to find the gRPC-related dependencies implicitly makes this harder.

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

the indexserver directive docs state that the directive is deprecated. :(

Ha, my bad.


Also, we could have worked around the issue better in tox.ini before #2308: relying on pip to find the gRPC-related dependencies implicitly makes this harder.

I'm not sure what you mean.

@tseaver
Copy link
Contributor

tseaver commented Sep 16, 2016

@dhermes #2308 removed the explicit [grpc]deps bit in tox.ini: that would have given us a place to ensure that the new dependencies were installed (in fact, without that section, the indexserver option would have been worthless).

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

@tseaver How is that different than putting them in setup.py?

@tseaver
Copy link
Contributor

tseaver commented Sep 16, 2016

In tox.ini, they get installed before google-cloud-python: the tox dependency syntax allows us to specify more options (e.g, URLs, local files) than can be spelled in install_requires. For instance, @bjwatson might have (temporarily) vendored in the sdists, and spelled installing them:`

[grpc]
deps =
    _vendor/grpc-spanner-v1-0.8.1.tar.gz
    _vendor/gapic-google-pubsub-v1-0.9.0b1.tar.gz
    _vendor/grpc-google-pubsub-v1-0.9.0b1.tar.gz
    _vendor/gapic-google-logging-v2-0.9.0b1.tar.gz
    _vendor/grpc-google-logging-v2-0.9.0b1.tar.gz

@bjwatson
Copy link
Author

@dhermes I thought that tox might have that feature, but I couldn't quickly find correct incantation.

Thanks for looking into the breakages. I'm at a hack-a-thon today for gcloud-java samples, but I can be pre-empted if there's anything I need to fix in my packages. I'd like to no longer be blocking your 0.19.0 release by EOD. PLMK.

@dhermes dhermes merged commit ca4f024 into googleapis:master Sep 16, 2016
@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

I just did a squash merge. No worries.

@bjwatson
Copy link
Author

Woo hoo!

@theacodes
Copy link
Contributor

Ignore clabot if you can manually verify all committers are under CLA.

@theacodes
Copy link
Contributor

(clabot can't handle multiple authors, it's a known issue)

@bjwatson
Copy link
Author

@dhermes Sorry, just saw your comments about squashing my commits. I didn't know that y'all kept PRs squashed. On GAPIC, we always wait until the merge-time to squash-and-commit using the commit button dropdown menu.

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

using the commit button dropdown menu.

  1. We prefer keeping the commits from PRs in history
  2. The squash commit isn't as easy to identify in history. Usually when making release notes we just go through all the merge commits created by PRs (not squashed regular commits).

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

No worries though

@bjwatson
Copy link
Author

I see, so you don't normally squash. This was just a one-off request due to multiple authors?

@dhermes
Copy link
Contributor

dhermes commented Sep 16, 2016

Looks like this broke some system tests. Working on a fix now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no This human has *not* signed the Contributor License Agreement. packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants