-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add support for Chalice #84
Conversation
2 similar comments
I fixed all of Travis' nagging since the last version. I have added a decorator for Chalice that will enable writing LTI applications in AWS Lambda. Chalice is similar to Flask for serverless applications. |
can you add unit tests? please check https://github.com/mitodl/pylti/tree/master/pylti/tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to update requirement.txt and test_requirement.txt Chalice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we refactor flask.py to reuse code, code seems identical
I found an issue in Chalice's local mode when writing test cases. That has been fixed and I can move forward again. See aws/chalice#765 for details. In addition to adding test cases, I have created a common base class that the flask and chalice decorators share in common.py. the base class includes all methods that are identical across implementations. Finally, I have merged the two commits that happened while I was waiting foe the Chalice team to incorporate my PR to fix local mode. |
please rebase it. |
please also add instructions to test it manually. |
New decorator for AWS Chalice. This can be used to build serverless LTI with AWS Lambda. Included test cases, merged with latest branch and squashed.
Squashed changes and rebased my branch. I also created a simple hello world project for testing. You can find that here. https://github.com/brianjbeach/lti-chalice-sample |
Hi. Just pinging this thread again hoping to move this forward. |
@brianjbeach I started taking a deeper look at this and it's a much larger set of changes than I was anticipating. How much of this is strictly necessary for Chalice support, as opposed to the specific test implementation? I ask because we are thinking about making this library more neutral about implementation and moving all the Flask details in our other repo. If/when we do that, we might pull out the Chalice stuff as well into it's own repo. We really appreciate your interest, so I'm inclined to merge this, as long as you understand we may go in a slightly different direction in the future. |
@brianjbeach i followed steps mentioned in https://github.com/brianjbeach/lti-chalice-sample Note: I installed you master branch in vendor using
|
@brianjbeach i am back from vacation. any thoughts on #84 (comment). Can i test it locally? Even debug is true it is now showing me trace. |
Amir, I finally got to look at this today and I was able to reproduce your error. I have never tested in local mode, but the unit tests leverage local mode and they are passing. I believe the issue here is that I am assuming the protocol will always be https and local mode is running http. Chalice local does not make the original url available. Therefore, on line 96 of chalice.py, I am building it. As you can see I have https hard coded. I opened an issue with chalice to have the original url added to the attribute collection. In the meantime can you test with lambda? I know you probably don't have an AWS account, but you should be able to create one and run this code for free. The first 1M invocations of Lambda are free each month. |
Amir, I updated the decorator to assume http (rather than https). It now works in local mode. I had to add a header to the tests to force them into https. |
i was trying but get error. can be configuration missing
can you give me time today where we can sit together on hangout and test it? |
Amir, I updated the instructions here https://github.com/brianjbeach/lti-chalice-sample. I have ran through that a few times now. What client are using for testing? I don't see the request body in the screenshot so it is difficult for me to debug it. |
…munication error is sent to the client.
Amir, I added an call to log the original exception with a stack trace whenever we return the default_error "There was an LTI communication error". This should help you see why you are failing. Note that default_error is common so Flask and other implementations will also log when this error is thrown. |
…entified in issue 85 of origial repo.
I realize I am adding lots of little changes again. Let me know if you want me to squash them again. |
@amir-qayyum-khan I think @blarghmatey was able to run this successfully? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
This is great. Thanks for all of you help. Have you pushed to pypi? |
Thank you for the reminder. I just pushed up v0.7.0 to PyPI as an sdist and a wheel |
I have added a decorator for Chalice that will enable writing LTI applications in AWS Lambda. Chalice is similar to Flask for serverless applications.