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

Drop support for Python < 3.6 #1812

Merged
merged 10 commits into from
Mar 21, 2021
Merged

Conversation

michael-k
Copy link
Contributor

@michael-k michael-k commented Oct 20, 2020

Most of the changes are replacing basestring with str, assertRaisesRegexp with assertRaisesRegex, and assertEquals with assertEqual and were done by 2to3.

I've excluded some of 2to3s useless changes that would just increase the PR even more. Like replacing for pk, pv in v.items(): with for pk, pv in list(v.items()):.

I had to adjust test_ref_hash method to get the tests to pass. I wondered why the tests on master didn't complain about it before and noticed, that only 109 tests are running in the Python 3 jobs on Travis (and on my machine). Compared to 447 in the Python 2 job.
With this PR all tests are running with Python 3.


First step towards #1558 (troposphere 3.0)

Closes #1091, #1295, #1299

@@ -381,7 +381,7 @@ def test_ref_eq(self):

def test_ref_hash(self):
s = hash("AWS::NoValue")
r = hash(Ref(s))
r = hash(Ref("AWS::NoValue"))
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 worked on Python 2, but failed on Python 3.

Python 2.7.18:

>>> hash("AWS::NoValue")
-6577466268859890267
>>> hash(hash("AWS::NoValue"))
-6577466268859890267

Python 3.8.6

>>> hash("AWS::NoValue")
-3204540641682287495
>>> hash(hash("AWS::NoValue"))
-898697632468593544
>>> hash(hash(hash("AWS::NoValue")))
-898697632468593544

ref #1053

@PatMyron
Copy link
Contributor

PatMyron commented Oct 20, 2020

Most of the changes [...] where done by 2to3.
I've excluded some of 2to3s useless changes

Might be nice to separate out manual changes and 2to3's automatic changes into separate commits:

gh pr checkout 1812 # https://github.com/cli/cli#installation
git checkout HEAD~
2to3 -w scripts/* troposphere/*                   # didn't look through other directories like examples/* tests/*
git remote add michael https://github.com/michael-k/troposphere
git diff michael/python3.6+ scripts/ troposphere/ # didn't look through other directories like examples/* tests/*
diff:
diff --git a/scripts/cfn b/scripts/cfn
index 82fa37d..750f52e 100755
--- a/scripts/cfn
+++ b/scripts/cfn
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 
-from __future__ import print_function
+
 import argparse
 import sys
 import time
diff --git a/scripts/cfn2py b/scripts/cfn2py
index 1b125fc..72e2d84 100755
--- a/scripts/cfn2py
+++ b/scripts/cfn2py
@@ -5,6 +5,11 @@ import argparse
 import json
 import pprint
 
+try:
+    getattr(__builtins__, 'basestring')
+except AttributeError:
+    str = str
+
 
 class object_registry(object):
     """Keep track of objects being created as Parameters or Resources
@@ -68,7 +73,7 @@ def do_header(d):
     if 'Resources' in d:
         seen = []
         resources = d['Resources']
-        for k, v in resources.items():
+        for k, v in list(resources.items()):
             (mod, tropo_object) = generate_troposphere_object(v['Type'])
             if tropo_object not in seen:
                 seen.append(tropo_object)
@@ -94,11 +99,11 @@ def do_description(d):
 def do_parameters(d):
     """Output the template Parameters"""
     params = d['Parameters']
-    for k, v in params.items():
+    for k, v in list(params.items()):
         object_name = objects.add(k)
         print('{} = t.add_parameter(Parameter('.format(object_name))
         print('    "{}",'.format(k))
-        for pk, pv in v.items():
+        for pk, pv in list(v.items()):
             print('    {}={},'.format(pk, output_value(pv)))
         print("))")
         print()
@@ -107,7 +112,7 @@ def do_parameters(d):
 def do_conditions(d):
     """Output the template Conditions"""
     conditions = d['Conditions']
-    for k, v in conditions.items():
+    for k, v in list(conditions.items()):
         print('t.add_condition("{}",'.format(k))
         print('    {}'.format(output_value(v)))
         print(")")
@@ -117,7 +122,7 @@ def do_conditions(d):
 def do_mappings(d):
     """Output the template Mappings"""
     mappings = d['Mappings']
-    for k, v in mappings.items():
+    for k, v in list(mappings.items()):
         print('t.add_mapping("{}",'.format(k))
         pprint.pprint(v)
         print(")")
@@ -144,7 +149,7 @@ def generate_troposphere_object(typename):
 
 def output_dict(d):
     out = []
-    for k, v in d.items():
+    for k, v in list(d.items()):
         out.append("{}={}".format(k.replace('\\', '\\\\'), output_value(v)))
     return ", ".join(out)
 
@@ -182,7 +187,7 @@ function_quirks = {
 
 def do_output_function(k, f, v):
     print('    {}={}('.format(k, f))
-    for pk, pv in v.items():
+    for pk, pv in list(v.items()):
         if pk in known_functions:
             do_resources_content(pk, pv, "")
         else:
@@ -194,7 +199,7 @@ def do_output_quirk_list(k, f, v):
     print('    {}=['.format(k))
     for e in v:
         print('    {}('.format(f))
-        for pk, pv in e.items():
+        for pk, pv in list(e.items()):
             if pk in known_functions:
                 do_resources_content(pk, pv)
             else:
@@ -205,7 +210,7 @@ def do_output_quirk_list(k, f, v):
 
 def do_output_quirk_mapping(k, v):
     m = function_quirks[k]
-    for pk in m.keys():
+    for pk in list(m.keys()):
         print('    {}={}('.format(k, pk))
         for e in m[pk]:
             print("        {},".format(output_value(v[e])))
@@ -214,7 +219,7 @@ def do_output_quirk_mapping(k, v):
 
 def do_output_quirk_metadata(k, v):
     m = function_quirks[k]
-    for pk in m.keys():
+    for pk in list(m.keys()):
         print('    Metadata={}('.format(pk))
         print("        {},".format(output_value(v)))
         print('    ),')
@@ -246,15 +251,15 @@ def do_resources(d):
     """Output the template Resources"""
 
     resources = d['Resources']
-    for k, v in resources.items():
+    for k, v in list(resources.items()):
         object_name = objects.add(k)
         (_, tropo_object) = generate_troposphere_object(v['Type'])
         if tropo_object in top_level_aliases:
             tropo_object = top_level_aliases[tropo_object]
         print('{} = t.add_resource({}('.format(object_name, tropo_object))
         print('    "{}",'.format(k))
-        for p in (x for x in ['Metadata', 'Properties'] if x in v):
-            for pk, pv in v[p].items():
+        for p in [x for x in ['Metadata', 'Properties'] if x in v]:
+            for pk, pv in list(v[p].items()):
                 if pk == "Tags":
                     print('    Tags=Tags(')
                     for d in pv:
@@ -333,7 +338,7 @@ def output_value(v):
 
     out = []
     # Should only be one of these...
-    for fk, fv in v.items():
+    for fk, fv in list(v.items()):
         if fk in function_map:
             (shortname, handler) = function_map[fk]
             if not isinstance(fv, list):
@@ -348,10 +353,10 @@ def do_outputs(d):
     """Output the template Outputs"""
 
     outputs = d['Outputs']
-    for k, v in outputs.items():
+    for k, v in list(outputs.items()):
         print('{} = t.add_output(Output('.format(k))
         print('    "{}",'.format(k))
-        for pk, pv in v.items():
+        for pk, pv in list(v.items()):
             if isinstance(pv, str):
                 print('    {}="{}",'.format(pk, pv))
             else:
@@ -385,7 +390,7 @@ if __name__ == "__main__":
     ]
 
     for s in sections:
-        if s in d.keys():
+        if s in list(d.keys()):
             globals()["do_" + s.lower()](d)
 
     do_trailer(d)
diff --git a/troposphere/__init__.py b/troposphere/__init__.py
index 3d7b815..dd1a281 100644
--- a/troposphere/__init__.py
+++ b/troposphere/__init__.py
@@ -61,8 +61,11 @@ def encode_to_dict(obj):
             new_lst.append(encode_to_dict(o))
         return new_lst
     elif isinstance(obj, dict):
-        return {name: encode_to_dict(prop) for name, prop in obj.items()}
+        props = {}
+        for name, prop in list(obj.items()):
+            props[name] = encode_to_dict(prop)
 
+        return props
     # This is useful when dealing with external libs using
     # this format. Specifically awacs.
     elif hasattr(obj, 'JSONrepr'):
@@ -114,13 +117,13 @@ class BaseAWSObject(object):
         self.__initialized = True
 
         # Check for properties defined in the class
-        for k, (_, required) in self.props.items():
+        for k, (_, required) in list(self.props.items()):
             v = getattr(type(self), k, None)
             if v is not None and k not in kwargs:
                 self.__setattr__(k, v)
 
         # Now that it is initialized, populate it with the kwargs
-        for k, v in kwargs.items():
+        for k, v in list(kwargs.items()):
             self.__setattr__(k, v)
 
         self.add_to_template()
@@ -152,7 +155,7 @@ class BaseAWSObject(object):
             raise AttributeError(name)
 
     def __setattr__(self, name, value):
-        if name in self.__dict__ \
+        if name in list(self.__dict__.keys()) \
                 or '_BaseAWSObject__initialized' not in self.__dict__:
             return dict.__setattr__(self, name, value)
         elif name in self.attributes:
@@ -253,18 +256,18 @@ class BaseAWSObject(object):
         if self.properties:
             return encode_to_dict(self.resource)
         elif hasattr(self, 'resource_type'):
-            return {
-                k: v
-                for k, v in self.resource.items()
-                if k != 'Properties'
-            }
+            d = {}
+            for k, v in list(self.resource.items()):
+                if k != 'Properties':
+                    d[k] = v
+            return d
         else:
             return {}
 
     @classmethod
     def _from_dict(cls, title=None, **kwargs):
         props = {}
-        for prop_name, value in kwargs.items():
+        for prop_name, value in list(kwargs.items()):
             try:
                 prop_attrs = cls.props[prop_name]
             except KeyError:
@@ -305,7 +308,7 @@ class BaseAWSObject(object):
         return cls._from_dict(title, **d)
 
     def _validate_props(self):
-        for k, (_, required) in self.props.items():
+        for k, (_, required) in list(self.props.items()):
             if required and k not in self.properties:
                 rtype = getattr(self, 'resource_type', "<unknown type>")
                 title = getattr(self, 'title')
@@ -553,9 +556,7 @@ class Tags(AWSHelperFn):
                 elif isinstance(arg, dict):
                     tag_dict.update(arg)
                 else:
-                    raise TypeError(
-                        "Tags needs to be either kwargs, dict, or AWSHelperFn"
-                    )
+                    raise TypeError
 
         def add_tag(tag_list, k, v):
             tag_list.append({'Key': k, 'Value': v, })
@@ -565,7 +566,7 @@ class Tags(AWSHelperFn):
             for k, v in sorted(tag_dict.items()):
                 add_tag(self.tags, k, v)
         else:
-            for k, v in tag_dict.items():
+            for k, v in list(tag_dict.items()):
                 add_tag(self.tags, k, v)
 
     # allow concatenation of the Tags object via '+' operator
@@ -883,7 +884,7 @@ class Parameter(AWSDeclaration):
                 allowed = [float, int]
                 # See if the default value can be coerced into one
                 # of the correct types
-                if not any(check_type(x, default) for x in allowed):
+                if not any([check_type(x, default) for x in allowed]):
                     raise ValueError(error_str %
                                      (param_type, type(default), default))
             elif param_type == 'List<Number>':
@@ -894,7 +895,7 @@ class Parameter(AWSDeclaration):
                 dlist = default.split(",")
                 for d in dlist:
                     # Verify the split array are all numbers
-                    if not any(check_type(x, d) for x in allowed):
+                    if not any([check_type(x, d) for x in allowed]):
                         raise ValueError(error_str %
                                          (param_type, type(d), dlist))

@michael-k
Copy link
Contributor Author

Might be nice to separate out manual changes and 2to3's automatic changes into separate commits

Sure, I'll try to make it easier to review this PR. (Later today, it's already past midnight here ;) )

@michael-k
Copy link
Contributor Author

I've the changes up in smaller commits. The first one is still pretty massive. It might be best to run 2to3 -n -w --no-diffs . on the current master and check the diff between that and my first commit. Just to make sure that I didn't smuggle in any changes. And then review the other commits one by one.

@markpeek
Copy link
Member

@michael-k per your comment in the awacs Python3 PR, here's my plan:
There are a few more changes I want to add to troposphere for the final Python2 release. Likely done this weekend. I'll then do a final Python 2 release of 2.7.0. After that release I was going to switch the project over to Python3 only and do a 3.0.0 major release. Per your offer, let me ping back on this PR when I'm ready for the Python3 changes and you can bring this PR up to date.

After this PR has been landed, I'll look at the black conversion and switching the repo from master to main.

@markpeek
Copy link
Member

Release 2.7.0 has been released and should be the last Python 2.x release. You can now rebase these changes onto it. Thanks.

@michael-k
Copy link
Contributor Author

@markpeek done. The PR is ready for review :)

@markpeek markpeek merged commit 5ec03f3 into cloudtools:master Mar 21, 2021
@markpeek
Copy link
Member

Awesome. Thanks @michael-k!

@michael-k michael-k deleted the python3.6+ branch March 21, 2021 19:16
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.

tests fail on Python 3 because of 'basestring'
3 participants