-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Deprecate: Remove Python2.7 installation support #1416
Conversation
Nice 🔥🔥, next up all of the six stuff, six.moves etc. |
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.
Minor questions, totally awesome work!
requirements/dev.txt
Outdated
pytest==5.1.2 | ||
#mock==2.0.0 # mock is now part of the Python standard library, available as unittest.mock in Python 3.3 onwards. |
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.
Should we remove this completely?
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.
O yes this should be.
|
||
class InvalidLayerVersionArn(UserException): | ||
""" | ||
The LayerVersion Arn given in the template is Invalid | ||
""" | ||
|
||
pass | ||
|
||
|
||
class UnsupportedIntrinsic(UserException): |
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.
We dont need a pass anymore? Didnt know this. Good to know.
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.
@@ -152,7 +154,7 @@ def _download_swagger(self, location): | |||
""" | |||
|
|||
if not location: | |||
return |
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.
Are we just being explicit? or is this the norm with Python3?
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.
This falls under pylint's inconsistent-return-statements (R1710), see http://pylint.pycqa.org/en/latest/technical_reference/features.html for more details.
I ended up just updating everything to make this go away. We could instead ignore this and revert the commit with all these types of changes.
else: | ||
LOG.debug("Builder crashed") | ||
raise ValueError(msg) | ||
LOG.debug("Builder crashed") |
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.
Not blocking on this, but we have the same verbiage on the 'builder crashed' on if there was an exception or if there was error code we dont understand. We need to differentiate on that.
@@ -1,3 +1,4 @@ | |||
# -*- coding: utf-8 -*- |
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.
probably not needed.
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.
Yes I removed that but forgot to commit it to see if windows would pass.
Also could you remove: https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/cli/command.py#L27 as well :) |
Good call. |
This reverts commit 6e40c34af8e814d260eb3747ab11fecddad31032.
byte_stdout = sys.stdout.buffer # pylint: disable=no-member | ||
|
||
return byte_stdout | ||
return sys.stdout.buffer |
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.
Yaay!
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.
Py3 🐍🐍🐍 !!
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.
YEEAAAAAHH!
This reverts commit 360d90a. This commit originally reverted aws#1140 due to os.symlink not being available on windows in Python2.7 stdlib, details here: aws#1315 (comment). We recently removed Python2.7 support in aws#1416, so this commit is a revert of the revert, with some additional black formating to make `make pr` pass.
Issue #, if available:
#1273
Description of changes:
Removes the ability to install SAM CLI into a Py2 environment.
I want to take this time to also upgrade some of our dev dependencies that were stuck in the stone age and get rid of six. We will need to do similar things in
aws_lambda_builders
but that will be completely separate than this. If some of the dev dependency upgrades become to much, I will move that to a different PR but for now keeping this as a Draft PR.Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.