Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Django samples #636

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Django samples #636

merged 1 commit into from
Sep 20, 2016

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Aug 20, 2016

No description provided.

# Django Samples

These two sample Django apps provide a skeleton for the two main use cases of the
Django contrib helpers

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@waprin
Copy link
Contributor Author

waprin commented Aug 23, 2016

@nathanielmanistaatgoogle responded to all comments but need to take another pass myself but have to run at this minute, will finish up later tonight. Will also stop importing functions across the samples though as discussed debatable whether it makes more sense to stick with Django conventions over oauth2client conventions.

@waprin
Copy link
Contributor Author

waprin commented Aug 23, 2016

@nathanielmanistaatgoogle ok ready for another pass, also cleaned up some unnecessary files. Following the style guide for imports does make sense to me, I think it's just the types in settings.py we should leave alone.

Whenever review is LGTM, besides needing to squash the review commits, I'd like to do a final pass on an actual server for both these samples before merge.

@cloud-dpe-jenkins
Copy link

Starting a new build on Jenkins!

@cloud-dpe-jenkins
Copy link

Build on Jenkins Started!

@cloud-dpe-jenkins
Copy link

Build finished. No test results found.

@waprin
Copy link
Contributor Author

waprin commented Aug 24, 2016

Sorry experimenting with Jenkins job to report system tests in background.

@@ -0,0 +1,3 @@
Django==0.10.0

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1,3 @@
Django==1.10.0
oauth2client==3.0.0

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

Completed another review pass; I don't think I noticed anything beyond cosmetics.



@decorators.oauth_required
def get_profile_required(request):

This comment was marked as spam.

This comment was marked as spam.

@wasabigeek
Copy link

@waprin did you have the chance to take a look at the comments?

@waprin
Copy link
Contributor Author

waprin commented Sep 14, 2016

@wasabigeek was on vacation from work and programming in general, will look back around on this tonight!

@waprin waprin force-pushed the django_samples branch 2 times, most recently from 9fee377 to a917acf Compare September 16, 2016 22:45
Contains two sets of samples - one for the
“Google auth” system and one for the “Django
user auth” system.
@waprin
Copy link
Contributor Author

waprin commented Sep 19, 2016

@wasabigeek @nathanielmanistaatgoogle thanks for review comments, I addressed all the issues and tested them again, ready for review pass. Prob should wait until a release before we merge though since the samples working do depend on some of the bugfixes.

@nathanielmanistaatgoogle
Copy link
Contributor

I'm satisfied. Who else will be reviewing this?

@waprin
Copy link
Contributor Author

waprin commented Sep 20, 2016

I think we had an oauth2client maintainer and someone who tried the samples (thanks again @wasabigeek ) so we should be good to merge. I will also at some point take a look at the google-api-python-client and see if it makes sense to redirect here which should get us more eyes. But since the samples will have known bug fixes until the next release, maybe @jonparrott wants to merge whenever next release is cut (after we update the requirements.txt)?

@theacodes
Copy link
Contributor

@waprin the next release is cut for what? this library? What changes are needed?

@waprin
Copy link
Contributor Author

waprin commented Sep 20, 2016

@jonparrott #623 #651 #614 . Otherwise the samples are unfortunately probably too buggy to even have in the repo.

@theacodes
Copy link
Contributor

I'm going to merge this and cut a new release.

@theacodes theacodes merged commit 3f9fdbd into googleapis:master Sep 20, 2016
@theacodes
Copy link
Contributor

Actually, I can't release without bumping a major version which we wanted to reserve for the transport refactors.

@waprin where are these samples referenced?

@waprin
Copy link
Contributor Author

waprin commented Sep 20, 2016

@jonparrott they are only referenced here for now

@theacodes
Copy link
Contributor

@waprin then I am okay with the situation for now. Let's hold off on any external references until we can release 4.0.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants