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

more faithfully represent aws lambda python runtime context #5291

Merged

Conversation

softprops
Copy link
Contributor

@softprops softprops commented Sep 12, 2018

What did you implement:

Closes #5283

Brought the python fake local invoke context up to greater parity with aws python runtime context based on the investigation in missing attributes and attribute value type mismatches mentioned in #5283 (comment)

How did you implement it:

I added two new property methods to invoke.py for the two attributes I was missing and corrected the value type of memory_limit_in_mb to match that of the aws lambda python runtime

How can we verify it:

You can essentially follow the same experiment here #5283 (comment)

you can take the off the shelf aws lambda python3.6 runtime template and change it to output the attrs and value types defined on the context object

def hello(event, context):
    return [(attr, str(type(getattr(context, attr)))) for attr in dir(context) if not attr.startswith('__')]

the compare the output of serverless invoke local -f hello vs serverless invoke -f hello ( after deploying it of course )

you should see that the value type of memory_limit_in_mb is now consistent and the attributes log_group_name and log_stream_name now exist in both contexts

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@softprops
Copy link
Contributor Author

@eahefnawy thoughts on this?

@softprops
Copy link
Contributor Author

@horike37 / @RafalWilinski reaching out as you folks have ushered in the last few merges to this file. What are your thoughts on this change?


@property
def log_stream_name(self):
return '2018/09/12/[$' + self.version + ']58419525dade4d17a495dceeeed44708'
Copy link
Contributor

Choose a reason for hiding this comment

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

How about actual date? 😄

@RafalWilinski
Copy link
Contributor

Left one small comment, other than that, LGTM 💃

@softprops
Copy link
Contributor Author

@RafalWilinski, I pushed a change to address the comment. I used time.strftime since invoke.py was already using the time module

>>> from time import strftime
>>> strftime('%Y/%m/%d')
'2018/09/18'

Let me know what you think.

@horike37 horike37 added this to the 1.33.0 milestone Sep 19, 2018
Copy link
Contributor

@horike37 horike37 left a comment

Choose a reason for hiding this comment

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

@softprops
LGTM 👍

@horike37 horike37 merged commit 373efc4 into serverless:master Sep 19, 2018
@softprops
Copy link
Contributor Author

thank you so much @horike37! what does the release cycle look like for batching up changes?

@horike37
Copy link
Contributor

horike37 commented Sep 20, 2018

@softprops
You are welcome 😄 Thank you for this work well done 💯 Awesome!

what does the release cycle look like for batching up changes?

We don't decide a definite release cycle. But recent releases looks like every month.

@softprops
Copy link
Contributor Author

awesome. I noticed you folks have been releasing a lot lately. I love it!

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.

3 participants