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

Deprecate: Remove Python2.7 installation support #1416

Merged
merged 42 commits into from
Oct 23, 2019
Merged

Conversation

jfuss
Copy link
Contributor

@jfuss jfuss commented Sep 17, 2019

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:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sriram-mv
Copy link
Contributor

Nice 🔥🔥, next up all of the six stuff, six.moves etc.

@jfuss jfuss changed the title Remove/py2 Depreciate: Remove Python2.7 support Sep 18, 2019
@jfuss jfuss changed the title Depreciate: Remove Python2.7 support Depreciate: Remove Python2.7 installation support Sep 18, 2019
@jfuss jfuss marked this pull request as ready for review September 23, 2019 19:07
Copy link
Contributor

@sriram-mv sriram-mv left a 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!

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

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?

Copy link
Contributor Author

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):
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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")
Copy link
Contributor

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 -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

probably not needed.

Copy link
Contributor Author

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.

@sriram-mv
Copy link
Contributor

@jfuss
Copy link
Contributor Author

jfuss commented Sep 23, 2019

Also could you remove: https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/cli/command.py#L27 as well :)

Good call.

@sriram-mv sriram-mv changed the title Depreciate: Remove Python2.7 installation support Deprecate: Remove Python2.7 installation support Sep 23, 2019
byte_stdout = sys.stdout.buffer # pylint: disable=no-member

return byte_stdout
return sys.stdout.buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Yaay!

Copy link
Contributor

@sanathkr sanathkr left a comment

Choose a reason for hiding this comment

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

Py3 🐍🐍🐍 !!

Copy link
Contributor

@sriram-mv sriram-mv left a comment

Choose a reason for hiding this comment

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

YEEAAAAAHH!

@jfuss jfuss merged commit ee91629 into aws:develop Oct 23, 2019
jfuss added a commit to jfuss/aws-sam-cli that referenced this pull request Apr 15, 2020
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.
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.

None yet

3 participants