fix: Py27hash fix #2182
fix: Py27hash fix #2182
Conversation
| Our officially supported Python versions are 2.7, 3.6, 3.7 and 3.8. Follow the idioms from this [excellent cheatsheet](http://python-future.org/compatible_idioms.html) to | ||
| make sure your code is compatible with both Python 2.7 and 3 (>=3.6) versions. | ||
| Our CI/CD pipeline is setup to run unit tests against both Python 2.7 and 3 versions. So make sure you test it with both versions before sending a Pull Request. | ||
| Our officially supported Python versions are 3.6, 3.7 and 3.8. |
There was a problem hiding this comment.
Since we're deprecating python 2, and this PR removes tests from running in python 2, I decided to also remove all other references to it. Let me know if I should undo the changes to documentation in case we want to do them more formally later on.
There was a problem hiding this comment.
We should update our setup.py to constraint to specific py versions too.
There was a problem hiding this comment.
I did remove python 2.7 from setup.py in this PR, but I wasn't sure about Programming Language :: Python so that one is still in the file. Should I also remove it?
There was a problem hiding this comment.
I was thinking more like this: https://github.com/aws/aws-sam-cli/blob/develop/setup.py#L57 Maybe outside of this PR, but we don't actually constrain what python version are supported. So we can make the code none compatible but it could still be install-able through pip in python2.7
Codecov Report
@@ Coverage Diff @@
## develop #2182 +/- ##
===========================================
+ Coverage 93.58% 94.39% +0.81%
===========================================
Files 90 97 +7
Lines 6124 7084 +960
Branches 1260 1433 +173
===========================================
+ Hits 5731 6687 +956
+ Misses 183 181 -2
- Partials 210 216 +6
Continue to review full report at Codecov.
|
| Our officially supported Python versions are 2.7, 3.6, 3.7 and 3.8. Follow the idioms from this [excellent cheatsheet](http://python-future.org/compatible_idioms.html) to | ||
| make sure your code is compatible with both Python 2.7 and 3 (>=3.6) versions. | ||
| Our CI/CD pipeline is setup to run unit tests against both Python 2.7 and 3 versions. So make sure you test it with both versions before sending a Pull Request. | ||
| Our officially supported Python versions are 3.6, 3.7 and 3.8. |
There was a problem hiding this comment.
We should update our setup.py to constraint to specific py versions too.
samtranslator/model/apigateway.py
Outdated
| if self.identity.get("Context"): | ||
| identity_source_context = list(map(lambda c: "context." + c, self.identity.get("Context"))) | ||
| identity_source_context = [] | ||
| for c in self.identity.get("Context"): |
There was a problem hiding this comment.
I see a pattern across these functions. Can we abstract this into a function?
| + make_shorthand(function_arn) | ||
| + "/invocations" | ||
| ) | ||
| if function_arn.get("Fn::GetAtt") and isinstance(function_arn["Fn::GetAtt"][0], Py27UniStr): |
There was a problem hiding this comment.
Why only [0]? What happens if Fn::GetAtt:[] is defined?
There was a problem hiding this comment.
function_arn is always of the form {"Fn::GetAtt": ["<function_logical_id>", "Arn"]} as it's created from function.get_runtime_attr("arn"). We only want to check if the function logical id is a Py27UniStr instance.
| + make_shorthand(function_arn) | ||
| + "/invocations" | ||
| ) | ||
| if function_arn.get("Fn::GetAtt") and isinstance(function_arn["Fn::GetAtt"][0], Py27UniStr): |
There was a problem hiding this comment.
function_arn is always of the form {"Fn::GetAtt": ["<function_logical_id>", "Arn"]} as it's created from function.get_runtime_attr("arn"). So we only need to check Fn::GetAtt.
There was a problem hiding this comment.
That is a big assumption. It may be true today but a change could impact this code path unknowingly. I'll trust the team this is sufficient but intrinsic functions have bitten SAM so many times that checking like this seems risk. We should find ways to encapsulate this logic in some manner to reduce the intrinsic function reading spread throughout the code base.
samtranslator/parser/parser.py
Outdated
| if parameter_values is None: | ||
| raise ValueError("`parameter_values` argument is required") | ||
|
|
||
| self.validate_datatypes(sam_template) |
There was a problem hiding this comment.
self is weird to me here. I would assume if we are using self that validate_datatypes would not be a static method.
| @@ -0,0 +1,23 @@ | |||
| The AWS Serverless Application Model includes the following third-party software/licensing: | |||
There was a problem hiding this comment.
Is this file adding in our generated .whl files?
There was a problem hiding this comment.
No, will update it to include in the wheel
tox.ini
Outdated
| # and then run "tox" from this directory. | ||
|
|
||
| [tox] | ||
| envlist = py27, py36, py37, py38 |
There was a problem hiding this comment.
Missed that one, thank you
| editor.add_auth_to_method(api=api, path=self.Path, method_name=self.Method, auth=self.Auth) | ||
|
|
||
|
|
||
| def _build_apigw_integration_uri(function, partition): |
There was a problem hiding this comment.
didn't want to include partition as a parameter because I thought both ways would be equivalent, but a lot of tests failed when going with either way.
Issue #, if available:
Description of changes:
Py27hash fix changes + removing python 2 from testing (the fix doesn't work with python 2, plus it's being deprecated).
Description of how you validated changes:
Added unit tests, ran changes through test suite
Checklist:
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.