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

Add support for Chalice #84

Merged
merged 6 commits into from
Jul 18, 2018
Merged

Add support for Chalice #84

merged 6 commits into from
Jul 18, 2018

Conversation

brianjbeach
Copy link
Contributor

I have added a decorator for Chalice that will enable writing LTI applications in AWS Lambda. Chalice is similar to Flask for serverless applications.

@coveralls
Copy link

Coverage Status

Coverage decreased (-19.0%) to 79.455% when pulling 9f2cc97 on brianjbeach:master into 5088b1d on mitodl:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-19.0%) to 79.455% when pulling 9f2cc97 on brianjbeach:master into 5088b1d on mitodl:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-19.0%) to 79.455% when pulling 9f2cc97 on brianjbeach:master into 5088b1d on mitodl:master.

@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage increased (+0.4%) to 97.474% when pulling b2a56af on brianjbeach:master into 51e3124 on mitodl:master.

@brianjbeach
Copy link
Contributor Author

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.

@amir-qayyum-khan
Copy link
Contributor

amir-qayyum-khan commented Mar 2, 2018

can you add unit tests? please check https://github.com/mitodl/pylti/tree/master/pylti/tests

@amir-qayyum-khan amir-qayyum-khan self-requested a review March 2, 2018 12:21
@amir-qayyum-khan amir-qayyum-khan self-assigned this Mar 2, 2018
Copy link
Contributor

@amir-qayyum-khan amir-qayyum-khan left a 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

Copy link
Contributor

@amir-qayyum-khan amir-qayyum-khan left a 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

@brianjbeach
Copy link
Contributor Author

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.

@amir-qayyum-khan
Copy link
Contributor

please rebase it.

@amir-qayyum-khan
Copy link
Contributor

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.
@brianjbeach
Copy link
Contributor Author

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

@brianjbeach
Copy link
Contributor Author

Hi. Just pinging this thread again hoping to move this forward.

@pdpinch
Copy link
Member

pdpinch commented Jun 6, 2018

@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.

@amir-qayyum-khan
Copy link
Contributor

amir-qayyum-khan commented Jun 14, 2018

@brianjbeach i followed steps mentioned in https://github.com/brianjbeach/lti-chalice-sample
i tried to access hello world api bit it giving me 500 error with no trace. though app.debug = True is set.
I did not deploy it instead I ran locally by this chalice local. I also tried to change port.
cc https://github.com/aws/chalice#tutorial-local-mode

Note: I installed you master branch in vendor using pip install git+https://github.com/brianjbeach/pylti.git@master -t ./vendor/

["There was an LTI communication error", 500]

screen shot 2018-06-14 at 5 21 44 pm

@amir-qayyum-khan
Copy link
Contributor

@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.

@brianjbeach
Copy link
Contributor Author

brianjbeach commented Jun 24, 2018

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.

@brianjbeach
Copy link
Contributor Author

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.

@amir-qayyum-khan
Copy link
Contributor

amir-qayyum-khan commented Jun 25, 2018

i was trying but get error. can be configuration missing

["There was an LTI communication error", 500]

can you give me time today where we can sit together on hangout and test it?

@brianjbeach
Copy link
Contributor Author

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.

@brianjbeach
Copy link
Contributor Author

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.

@brianjbeach
Copy link
Contributor Author

I realize I am adding lots of little changes again. Let me know if you want me to squash them again.

@pdpinch
Copy link
Member

pdpinch commented Jul 17, 2018

@amir-qayyum-khan I think @blarghmatey was able to run this successfully?

Copy link
Contributor

@amir-qayyum-khan amir-qayyum-khan left a 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

@blarghmatey blarghmatey merged commit 459ab01 into mitodl:master Jul 18, 2018
@brianjbeach
Copy link
Contributor Author

This is great. Thanks for all of you help. Have you pushed to pypi?

@blarghmatey
Copy link
Member

Thank you for the reminder. I just pushed up v0.7.0 to PyPI as an sdist and a wheel

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

Successfully merging this pull request may close these issues.

5 participants