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

Migrating utility methods from django-gapi-hooked #122

Merged
merged 2 commits into from Dec 12, 2019

Conversation

abrahamvarricatt
Copy link
Contributor

@abrahamvarricatt abrahamvarricatt commented Dec 10, 2019

Why are these methods being moved here?
They exist in another project which has Django as a dependency. We have a non-Django app which uses these methods and it feels like an over-kill to also include Django libraries when we don't need them.

Will these methods be removed from the other project? i.e. django-gapi-hooked ?
Not any time soon. After this PR is merged, I might add a note/comment in the code, but no plans to delete it right now. This is because I don't want to break any other project accidentally.

Will the documentation be updated to reflect the existence of these methods here?
Yes. At least, I'm looking to update these pages;
https://developers.gadventures.com/docs/webhooks.html#registering-a-webhook
https://developers.gadventures.com/docs/webhooks.html#verifying-a-webhook
after this PR is merged.

@jonprindiville
Copy link
Member

FYI there are a couple very basic unit tests in django-gapi-hooked associated with this functionality. Basically just making sure that nothing explodes when given various combinations of bytes/unicodes for the request body and API key.

a bunch of methods from our private repositories

Nope, django-gapi-hooked is public. I thought you were moving it because d-g-h lists Django as a requirement and you don't want to pull that in to your non-Django app 🤔

@abrahamvarricatt
Copy link
Contributor Author

😳 Oh that one isn't private? Ok, news to me. It was kept as a dependency in our requirements-private.txt file and I didn't want to mention that reason here. But yes - the final goal is to remove Django as a dependency for another project.

@abrahamvarricatt
Copy link
Contributor Author

Ok; gimme a moment, will pull in those tests too.

Copy link
Contributor

@marz619 marz619 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@marz619 marz619 merged commit b0e2c9b into master Dec 12, 2019
marz619 added a commit that referenced this pull request Dec 12, 2019
* Adds the `slug` field to Tour Dossiers (#120)
* Adds webhook signature utility methods (#122)
@marz619 marz619 deleted the migrating_signature branch December 12, 2019 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants