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

Preventing crash when running on AWS Lambda #72

Merged
merged 8 commits into from
Jan 19, 2021

Conversation

Mueller-Patrick
Copy link
Contributor

AWS Lambda functions are run in a readonly directory, which causes an OSError with icalevents as described in #65. Catching this error and calling httplib2 without cache fixes the problem.

@Hultner
Copy link
Member

Hultner commented Sep 24, 2020

Could you add a test as well as a log entry when this happens?
Wouldn't want it to silently happen undetectable for users who do want the cache.

@Mueller-Patrick
Copy link
Contributor Author

To be honest, it's kinda hard to write an automated unit test for an absolute edge case with a readonly working directory. If you have an idea how this can be achieved easily, please give me a hint. As for the logging, of course its possible to add some print or stuff like that as icalevents doesnt seem to use the logging library yet. I'll do that in a few minutes.

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #72 (e7afade) into master (ce0581e) will increase coverage by 0.76%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   82.82%   83.58%   +0.76%     
==========================================
  Files           4        4              
  Lines         326      329       +3     
  Branches       77       77              
==========================================
+ Hits          270      275       +5     
+ Misses         31       29       -2     
  Partials       25       25              
Impacted Files Coverage Δ
icalevents/icaldownload.py 78.57% <100.00%> (+6.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce0581e...e7afade. Read the comment docs.

@Hultner
Copy link
Member

Hultner commented Sep 24, 2020

To be honest, it's kinda hard to write an automated unit test for an absolute edge case with a readonly working directory. If you have an idea how this can be achieved easily, please give me a hint. As for the logging, of course its possible to add some print or stuff like that as icalevents doesnt seem to use the logging library yet. I'll do that in a few minutes.

Edge-cases like this is where tests are most important.

You could set up a test where you set the permission to read only (e.g. 550) and revert it after the test finished, during this test you would assert that the expected log message would be outputted.

It also would be beneficial to use the logging module over print so output can be configured by the user more easily.

@Mueller-Patrick
Copy link
Contributor Author

Well thats of course correct, but honestly I'm not going to change half of the whole project for a Pull Request that just adds half a line of code and is more of a hotfix for everyone that encounters this problem. The test should have already been in place before as I only added a catch for another error that is caused by the same issue. So I'm not going to be the one fixing the problems of others. I'm just a user that encountered this problem and wanted to provide the fix for everyone. If there is a problem with this approach of mine, feel free to close my PR as I already fixed the stuff sufficiently for me to use it in my projects. Everyone else, like John from #65, has to fix it for themselves then I guess.

@Hultner
Copy link
Member

Hultner commented Sep 28, 2020

No worries, I'm just a user as well. I'm just worried that if we don't make sure to improve the testing with every patch library will deteriorate, and I wouldn't wan't silent failures for things like non writable file systems.

@mapledyne
Copy link

This impacts Azure as well as AWS, and the fix looks like it would fix both.

@amcappelli
Copy link

I ran into this issue as well. Just posting my workaround in case it helps anyone. AWS Lambda provides a /tmp mount that is writable, so just change to that directory first.

os.chdir("/tmp")
calendar_events = icalevents.events(string_content=calendar, start=start, end=end)

@Hultner
Copy link
Member

Hultner commented Jan 16, 2021

I’m happy to merge this If someone is willing to write proper tests for the functionality.

@Mueller-Patrick
Copy link
Contributor Author

I'm working on a test right now as this seems to affect a lot of users, guess I'll get it done today

@Mueller-Patrick
Copy link
Contributor Author

Since the latest ci run failed, i looked into it and it seems that python needs sudo rights to perform os.chmod() commands. The travis config specifically says that the job should run without sudo. Not sure what to do here now.

@Hultner
Copy link
Member

Hultner commented Jan 16, 2021

Since the latest ci run failed, i looked into it and it seems that python needs sudo rights to perform os.chmod() commands. The travis config specifically says that the job should run without sudo. Not sure what to do here now.

Try using a tmp directory, should allow you to set whichever permissions. Chmod should work fine without sudo if you own the directory.

Copy link
Member

@Hultner Hultner left a comment

Choose a reason for hiding this comment

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

Looks great, good work with the test!

@Hultner Hultner merged commit faa1226 into jazzband:master Jan 19, 2021
@Hultner
Copy link
Member

Hultner commented Jan 19, 2021

@Mueller-Patrick I merged your change. I don't have the credentials to cut a new release on pypi so we'll have to wait on @irgangla for that.

capuanob pushed a commit to ennamarie19/icalevents that referenced this pull request Mar 6, 2023
Preventing crash when running on AWS Lambda
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.

4 participants