Closed bivald closed 7 years ago
instead of doing a merge, you should do a rebase
If I do, are Google interested in merging the pull request?
i can't speak for the maintainers of this repo, but the PR as-is shouldn't be merged imo
That's fine, the reason I'm asking is that since gcs-oauth2-boto-plugin chances rapidly any PR is soon outdated. Thus I'm happy to do a rebase and do it "properly" but it's unnecessary work to do it after every commit on master branch. So if any maintainer is looking, if you are interested in merging a py3 PR ping me on this thread and I'll fix everything.
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.
@jonparrott If you feel that this looks OK in terms of the actual code (when viewed on https://github.com/GoogleCloudPlatform/gcs-oauth2-boto-plugin/pull/18/files) I can create a new patch based on a diff between my fork and master as a single commit.
Background: I appologize that this pull request is a complete mess, I've spend an hour trying to rebase but rebasing through merges in different points in time is above my git skills I'm afraid. As a background this code was not intended as a pull request initially, but was simply used as an internal repo for python3 support with default .boto on GCE but has grown since. It should fix #10 #17 #21 (thanks to @romange).
These commits adds Python3 support, as well as continues on @romange work regarding oauth2client (added missing wrappers for when using json files as credentials)
I've confirmed that this works (simply using boto to download a file from GCS) manually on:
Python version | Credentials | oauth2client==1.5.2 | oauth2client==2.1.0 |
---|---|---|---|
2 | Default Service Account (on GCE) | works | works |
3 | Default Service Account (on GCE) | works | works |
pypy2 | Default Service Account (on GCE) | works | works |
2 | JSON credentials in boto.cfg | works | works |
3 | JSON credentials in boto.cfg | works | works |
pypy2 | JSON credentials in boto.cfg | works | works |
@bivald don't sweat git issues, we can now squash on merge so it's no big deal.
Looks okay to me, but @thobrla should be the one to merge.
In general we're amenable to this pull request, but I have a competing commit that adds 1.5.2 and >=2.0.0 compatibility.
In general, I think it would be best to separate out that compatibility and Python 3 compatibility into separate comments. Would you be willing to create a commit for just the Python 3 portion based on my oauth2client cross-compatibility commit? I intend to push that one within the next few days.
Absolutely, no problem. Merge that one and I can add py3 as a clean commit. If it's romanges, could you also look at:
Which complements that one for use with json files (Python 2)
On 10 juni 2016, at 20:36, thobrla notifications@github.com wrote:
In general we're amenable to this pull request, but I have a competing commit that adds 1.5.2 and >=2.0.0 compatibility.
In general, I think it would be best to separate out that compatibility and Python 3 compatibility into separate comments. Would you be willing to create a commit for just the Python 3 portion based on my oauth2client cross-compatibility commit? I intend to push that one within the next few days.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
Commit is now pushed, and 1.13 released: https://github.com/GoogleCloudPlatform/gcs-oauth2-boto-plugin/commit/6492539fc8eef91ca815e1710fffb1600756530e
Any updates on this PR?
Would be fine to merge a Python 3 support commit now that oauth2client 2 is supported, but this PR has not yet been updated.
@thobrla When do you see this merge happening?
As soon as the PR author (or another author) resolves the conflicts. It's unlikely the gsutil team will port this module themselves anytime soon.
(I'm the author) I'm afraid I haven't had the time to resolve the conflicts, and solved my problem my moving on from boto to using the gcloud-python cloud storage instead.
So there's good news and bad news.
:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.
:confused: The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.
Note to project maintainer: This is a terminal state, meaning the cla/google
commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.
@njainua it looks like the original author won't be updating this, so I'm going to close this. If you'd like to submit a pull request of your own based on this one we'd happily accept; otherwise we probably won't port this until gsutil supports Python 3.
for posterity, commit 9425f1dd80716888ae2a95b4d572de068f2f9c14 and such should address py3 compat
Gives the boto plugin python3 compatibility, as well as solve issue #17
All tests works, as well as my testing in real life, we're using this in production.